rust-lang / stdarch

Rust's standard library vendor-specific APIs and run-time feature detection
https://doc.rust-lang.org/stable/core/arch/
Apache License 2.0
611 stars 269 forks source link

Support different definitions of intrinsics based on LLVM versions #1189

Closed alexcrichton closed 3 years ago

alexcrichton commented 3 years ago

I don't think that this is currently supported by rustc today, but it's something I think that would be good to have. The implementation of all intrinsics is very closely tied to LLVM's expected codegen patterns for instructions, but these codegen patterns periodically change over time.

For example in LLVM 13 the codegen pattern expected for a number of WebAssembly intrinsics is changing, meaning that the patterns used for LLVM 12 will no longer be supported. I'd ideally like to land code ahead of time before the LLVM 13 update lands to support both methods of codegen (for 12 and 13) for wasm intrinsics. Otherwise this repository's CI will break as soon as the LLVM 13 update lands. Additionally it means that stdarch will only ever support one LLVM version at a time, even if rustc itself advertises support for multiple LLVM versions (e.g. if distros use LLVM 13 you'll have working wasm intrinsics but if they're stuck on, say, LLVM 12 then they won't have working wasm intrinsics).

This is relatively minor in that the main intrinsics, x86, seems to very rarely change codegen patterns. Wasm intrinsics are very new to LLVM and they were expected to have churn, so while I think that this would be good to add I don't think that there's a super pressing need for it.

Amanieu commented 3 years ago

Is this breakage simply due to LLVM generating different instructions than our tests expect, but which are functionally identical? I don't think that is much of a concern since we don't actually guarantee that any particular instruction is emitted.

If it's the LLVM intrinsic names (llvm.*) that have changed then that is more of an issue. I suppose we could support this by having build.rs detect the LLVM version of rustc and emit an appropriate cfg flag.

alexcrichton commented 3 years ago

At least for the upcoming wasm changes it's the latter of these, the llvm.* intrinsics that are wasm-specific are largely disappearing as they're replaced with more first-class codegen pattern matching.

I'd actually forgotten about build.rs, but that would require sync-ing I think the support between library/core/build.rs and crates/core_arch/build.rs I think? I'm not sure if it's worth doing all that just for some wasm intrinsics...

Lokathor commented 3 years ago

That would also be a problem if you're just using "a system llvm of an allowed version" instead of a bundled llvm, right?

bjorn3 commented 3 years ago

I assume the atomic and memory intrinsics are still supported and that only the simd intrinsics are the problem? I think part of the simd intrinsics can be implemented using the same simd_* platform intrinsics stdsimd uses and that are used elsewhere in stdarch.

https://github.com/rust-lang/stdarch/blob/62256316d93b9852badcf413e893501c83bfdc3b/crates/core_arch/src/wasm32/simd128.rs#L80-L248

llvm.wasm.anytrue.v16i8 could be simd_reduce_any. llvm.wasm.alltrue.v16i8 could be simd_reduce_all. The saturating add and sub could be simd_saturating_{add,sub}. llvm.wasm.bitmask.v8i16 could be simd_bitmask. The float rounding LLVM intrinsics also have simd_* platform intrinsics. There are LLVM intrinsics that need to be used, but simd_* platform intrinsics would cover most wasm intrinsics I think. It also has the advantage of not relying on pattern matching, thus improving performance for potential alternative backends.

alexcrichton commented 3 years ago

For wasm specifically there's a major difference between the bundled Rust LLVM 12 and stock LLVM 12. Stock LLVM 12 has incorrect encodings for wasm SIMD, and intrinsics we use probably aren't defined in stock LLVM 12. No one is, however, expecting stock LLVM 12 to generate valid wasm simd.

My motivation for opening this issue is the upcoming wasm simd changes in LLVM 13, but I think that this is a more general issue. For example some x86 intrinsics could change codegen over time, and we presumably want the intrinsic to still perform correctly regardless of whether LLVM version N or version N+1 is used. AFAIK this has happened very rarely, though. I don't know of at least any large-scale changes at any point of how LLVM implements intrinsics for x86.

For Wasm the purpose is to generate the actual wasm simd instruction, so the alternative @bjorn3 are not viable. LLVM would probably generate correct code but that's missing the point, I want to make sure LLVM actually generates the right instruction. The right instruction looks different in Rust's LLVM 12 and stock LLVM 13 (which is still in the works). I'd like to make a commit to the simd128.rs file which uses #[cfg] to select the right implementation, but such a #[cfg] does not exist. I assumed that this would need to be some unstable rustc feature, but it's a good point that build.rs could work as well.

All that being said, I think the absolute best thing we could do here would be to provide both the LLVM version and whether or not "rustllvm" is used (e.g. whether or not Rust's fork of LLVM is used). That would enable stdarch to have complete information about how to codegen intrinsics. For example with stock LLVM 12, if we cared which I don't think we need to, we could get most wasm simd intrinsics working rather than just importing intrinsics which don't exist in LLVM 12. With the version information we could also generate new code for LLVM 13.

bjorn3 commented 3 years ago

For Wasm the purpose is to generate the actual wasm simd instruction, so the alternative @bjorn3 are not viable.

Why would simd_* not generate the right instruction, but a scalar emulation that can be pattern matched would? If anything I would expect simd_* to be more robust as it doesn't rely on pattern matching. For exanple the pattern matching won't happen in debug mode at all, right?

LLVM would probably generate correct code but that's missing the point, I want to make sure LLVM actually generates the right instruction.

There are already disassembly checks like #[cfg_attr(test, assert_instr(v128.store))].

Amanieu commented 3 years ago

For Wasm the purpose is to generate the actual wasm simd instruction, so the alternative @bjorn3 are not viable. LLVM would probably generate correct code but that's missing the point, I want to make sure LLVM actually generates the right instruction.

I strongly disagree: the intrinsics in std::arch specifically make no guarantee that any particular instruction is emitted. If, for some reason, you absolutely need a particular instruction then you should use asm! which does have this guarantee (asm! is supported on wasm).

The disassembly checks that we use in stdarch CI are primarily intended as sanity checks, not hard guarantees on codegen.

alexcrichton commented 3 years ago

Hm ok, this seems like the discussion is mostly "how do we not do this", so if there's no interested I'll just deal with problems when they happen.