Skip to content

Conversation

@SparrowLii
Copy link
Member

@SparrowLii SparrowLii commented May 17, 2021

This PR revised the implementation of the d_s64 suffix neon instructions to be consistent with Clang

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@SparrowLii
Copy link
Member Author

SparrowLii commented May 17, 2021

I think vshl may have a bug in llvm. It is compiled correctly in godbolt but it failed in CI:
LLVM ERROR: Cannot select: 0x7fb987899d00: i64 = AArch64ISD::VSHL Constant:i64<1>, Constant:i32<2>, crates/core_arch/src/aarch64/neon/generated.rs:7874:5 @[ crates/core_arch/src/aarch64/neon/generated.rs:15577:32 @[ crates/core_arch/src/aarch64/neon/generated.rs:15572:5 @[ crates/core_arch/src/aarch64/neon/generated.rs:15572:5 @[ /rustc/fe72845f7bb6a77b9e671e6a4f32fe714962cec4/library/core/src/ops/function.rs:227:5 ] ] ] ] 0x7fb985815750: i64 = Constant<1> 0x7fb9d0d1e410: i32 = Constant<2> In function: _ZN4core3ops8function6FnOnce9call_once17h1592c9ba5b3050ecE

let a: int64x1_t = transmute(a);
let b: int64x1_t = transmute(b);
simd_extract(simd_add(a, b), 0)
a + b
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use wrapping_add

@Amanieu
Copy link
Member

Amanieu commented May 17, 2021

You can reproduce the error in godbolt by adding a test case which uses constant inputs: https://rust.godbolt.org/z/nPq57xxso

I can think of 2 ways to fix this:

  • Just use normal shift instructions instead of intrinsics.
  • Use int64x1_t for the intrinsic arguments instead of i64.

@SparrowLii
Copy link
Member Author

SparrowLii commented May 18, 2021

Yes, it seems that both i64 and int64x1_t will get the same assembly instructions for vshl. My thought is to use the same link as Clang, so that llvm can be optimized to get the desired result. (for example, we would like srsra instruction in vrsrad_n_s64). And it does work

@SparrowLii
Copy link
Member Author

It seems that there is something wrong with Cirrus CI, can we fix it?

error: error: 'sysinfo not supported on this platform'
info: installing component 'clippy'
error: error: 'sysinfo not supported on this platform'
info: installing component 'rust-std'
error: error: 'sysinfo not supported on this platform'

@Amanieu Amanieu merged commit a2e8b9a into rust-lang:master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants