LLVM produces SIGILL when enabling avx2 target feature on x86_64-unknown-none
#117938
Comments
rustbot
added
the
needs-triage
|
With assertions: |
|
The combination of |
|
@nikic #[target_feature(enable = "avx2")]
unsafe fn simd_fn() { ... }
if false {
unsafe { simd_fn() }
} else {
soft_fn();
}The idea here is that the first branch should be eliminated by compiler. But it looks like the compiler starts compiling and lowering |
|
The compiler doesn't eliminate branches like these in debug mode right now (which, while it does cause perf problems sometimes, is not a bug). |
|
Yes, but note that the SIGILL happens with |
|
I checked, and the |
|
Can Rust remove |
|
That should be possible, with two caveats:
cc @RalfJung I'm sure you will appreciate this new bit of target feature fun. |
|
As noted in the linked RustCrypto issue, ideally we need something like this to properly handle nastiness like this in libraries. But I guess it's a separate discussion. |
|
I'd say compiling SIMD code on a softfloat target makes fairly little sense. Maybe we should have Letting you disable target features in a function wouldn't really help, we'd still want to reject even declaring such a function since its ABI is all wrong. (Or we'd have to get LLVM to support a softfloat ABI for SIMD types I guess.) See rust-lang/lang-team#235 for more details on the ABI issues surrounding target features. We definitely do not want to support |
Yes, it could work, but I think a more fundamental solution would be a proper support of "negative" target features. Arguably, we should consider the relation between hard floats and SIMD instructions nothing more than an implementation detail of the x86 targets. Another alternative is to replace SIMD functions (i.e. functions marked with |
But what do you want to do with them? I don't think there is any way we can accept a Maybe after fixing all the LLVM issues around this we could accept this and give it a softfloat ABI. But that's far off. And even then I'm not sure it is desired; in some cases people compiling for softfloat targets want to be really sure that the hardfloat registers are not used, since they plan to not save/restore them on context switches. Enabling hardfloat mode on a softfloat target is unsound in such situations. Hardfloat/softfloat isn't just a regular target feature you can switch locally. It's a global decision. |
|
Note that this issue impacts curve25519-dalek as well: dalek-cryptography/curve25519-dalek#601 |
|
|
Yeah... I just wasn't able to come up with a better alternative yet. Maybe we should declare (and have the feature-detect macros implement) that SSE features are never available on softfloat targets. Then we can compile functions with SSE |
|
I think that's what @newpavlov was suggesting earlier. Sounds good to me. Edit: specifically meant the second paragraph of #117938 (comment) |
I am talking about this proposal. Right now this part of target specification:
from library point of view only means that the SIMD features are not enabled. Thus code which supports runtime detection of target features could assume that during execution SIMD features may be available, so it has to keep SIMD detection and SIMD-optimized branches. In other words, right now Rust provides only two target feature states Enabling soft floats would automatically make all SIMD features like SSE and AVX "negative", thus in libraries we will be properly to |
saethlin
added
T-compiler
Ah yes they were. I somehow missed that -- sorry. Great, good to see designs converge :)
I don't understand what you mean. I thought you were suggesting In terms of enabled target features, there's the set of target features that are statically enabled in the current code (via (I don't think it is sound to use any other way of querying for runtime target features, it must be our macros. I hope cpufeatures uses those macros internally.) |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Anyway this is getting highly speculative and it's discussing a significant language extension. If we want to continue discussing this we should start a new thread. It's not really relevant for this issue. The core of this issue is that enabling certain features on certain targets just doesn't work currently, which leads to portability issues. |
RalfJung
changed the title
[email protected] to x86_64-unknown-nonex86_64-unknown-none
We want to make sure they aren't used unless specifically requested.
Not necessarily. The Linux kernel has I do agree that The discussion of "negative" features is a totally separate thing. What's really happening is that the crypto libraries are using CPUID/_xgetbv to detect CPU features, and if CPUID/_xgetbv says some CPU feature is available, then they feel free to use it. This is not the correct thing for us to be doing on these -none targets and it's not something for the language team to solve. It should be tracked in a separate issue; there's already #60123 (comment) where I point out why the proposed |
That needs work on the LLVM side then, since currently this is not supported in LLVM. |
|
And that also complicates the ABI story. Given the principle that target features may not affect ABI (rust-lang/lang-team#235), I guess we need to
I don't know if LLVM supports the first point here. If the goal is to eventually allow using such target features on softfloat targets, then we should reject these target features until we have a way to enable them without affecting ABI. But that would mean there is no portable way to write code like what triggered this issue until LLVM is fixed... The fact that LLVM ties together "target features used by ABI" and "target features used by codegen" makes this hard to support. But I don't think we should compromise on having a consistent ABI within any given target triple. |
Maybe it already supports this, since clang can build the Linux kernel? Or maybe they only do their SIMD stuff in external .S files? |
|
Would CPUID-gating + |
Not quite. In the |
|
It may be a regression from 1.69 to 1.70. I was unable to trigger SIGILL on a simplified example, but this snippet gets properly compiled on 1.69, but causes |
Realistically, even if this were to be fixed in Rust 1.75, we'd need to find a workaround to avoid increasing MSRV for our projects to 1.75 (for these targets). It seems instead we may need to "just" ensure that all the conditional logic that enables use of vector registers happens at So, I see this being two issues:
WDYT? |
Steps to reproduce
Running
gdb --args $RUSTC_INVOCATION_PRINTED_BY_CARGOproduces this backtrace:Stable Backtrace
Using
nightly-2023-11-15toolchain produces a "LLVM ERROR" instead:Unless the
--releaseflag is used, then you get the SIGILL with the nightly toolchain. The backtrace appears to be similar to the stable toolchain one:Nightly Backtrace
Meta
Downstream discussion: RustCrypto/universal-hashes#189
rustc +1.73.0 --version --verboserustc +nightly-2023-11-15 --version --verboseThe text was updated successfully, but these errors were encountered: