-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert #132662 to fix regression on targets without f128 support #133037
Conversation
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @rust-lang/wg-const-eval |
Thanks for putting the revert up. Ideally we would fix this in the compiler... @bors r+ |
@bors r- I don't think we should revert a tier 1 perf fix in favor of a tier 3 target. |
The deal with tier 3 targets is that they can tag along as long as they don't cause problems for our primary targets. But when a target needs a specific work-around, that needs to be done without undue cost for tier 1 targets. |
Also, f16_f128 is a nighty feature, and it shouldn't unduly impact stable releases either. Fixing this in the compiler isn't even very hard. Somewhere around here, we can iterate the function arguments, and if any of them has type |
@RalfJung This is actually not tier 3 targets only. At least one of tier 2 target is also affected. $ cargo build -Z build-std=core --target arm64ec-pc-windows-msvc
Compiling core v0.0.0 (/Users/taiki/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core)
Compiling compiler_builtins v0.1.138
Compiling rustc-std-workspace-core v1.99.0 (/Users/taiki/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/rustc-std-workspace-core)
error: could not compile `core` (lib)
rustc-LLVM ERROR: Only 32 and 64 bit floating points are supported for ARM64EC thunks As said in #133035:
|
The list of T1 and T2 targets that have either a selection or link failure is pretty extensive (including arm64ec, s390x, sparc, powerpc, wasm, anything cranelift), I didn't realize that would be re-exposed via #132662. It seems worth reverting, adding the compiler automatic inline (I'll work on that now) and then re-landing. |
I'm not sure if this really works; My understanding is that normal
|
But only with |
|
I have a wip for the compiler change at #133050 |
There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains `f16` or `f128`. The cranelift backend also does not support these types. To work around this, every function in `std` or `core` that contains these types must be marked `#[inline]` in order to avoid sending any code to the backend unless specifically requested. However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return `f16` or `f128`. Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library. Fixes: rust-lang#133035 Closes: rust-lang#133037
There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains `f16` or `f128`. The cranelift backend also does not support these types. To work around this, every function in `std` or `core` that contains these types must be marked `#[inline]` in order to avoid sending any code to the backend unless specifically requested. However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return `f16` or `f128`. Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library. Fixes: rust-lang#133035 Closes: rust-lang#133037
Always inline functions signatures containing `f16` or `f128` There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains `f16` or `f128`. The cranelift backend also does not support these types. To work around this, every function in `std` or `core` that contains these types must be marked `#[inline]` in order to avoid sending any code to the backend unless specifically requested. However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return `f16` or `f128`. Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library. Fixes: rust-lang#133035 Closes: rust-lang#133037
#133050 is in the queue now, so this one can be closed. Thanks all! |
Rollup merge of rust-lang#133050 - tgross35:inline-f16-f128, r=saethlin Always inline functions signatures containing `f16` or `f128` There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains `f16` or `f128`. The cranelift backend also does not support these types. To work around this, every function in `std` or `core` that contains these types must be marked `#[inline]` in order to avoid sending any code to the backend unless specifically requested. However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return `f16` or `f128`. Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library. Fixes: rust-lang#133035 Closes: rust-lang#133037
This reverts #132662.
See #133035 for details of the regression.
Fixes #133035
r? @tgross35 @RalfJung