Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Nov 14, 2024

This reverts #132662.

See #133035 for details of the regression.

Fixes #133035

r? @tgross35 @RalfJung

…g, r=tgross35"

This reverts commit 8adb4b3, reversing
changes made to a00df61.
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2024

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 14, 2024
@tgross35
Copy link
Contributor

Thanks for putting the revert up. Ideally we would fix this in the compiler...

@bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2024

📌 Commit 0a370d8 has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2024
@RalfJung
Copy link
Member

@bors r-

I don't think we should revert a tier 1 perf fix in favor of a tier 3 target.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 14, 2024
@RalfJung
Copy link
Member

RalfJung commented Nov 14, 2024

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.

@RalfJung
Copy link
Member

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 f16 or f128 we can mark the function as inline. That's not a complete fix (e.g. when a float is wrapped in a newtype), but should be enough for this issue.

@taiki-e
Copy link
Member Author

taiki-e commented Nov 14, 2024

@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:

I suspect the same problem exists with the other targets listed here: https://github.com/rust-lang/compiler-builtins/blob/a09218f1c4d23ffbd97d68f0fefb5feed2469dc5/configure.rs#L82-L91

@tgross35
Copy link
Contributor

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.

@taiki-e
Copy link
Member Author

taiki-e commented Nov 14, 2024

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 f16 or f128 we can mark the function as inline. That's not a complete fix (e.g. when a float is wrapped in a newtype), but should be enough for this issue.

I'm not sure if this really works; My understanding is that normal #[inline] only works if optimization is enabled, but as mentioned in #133035, I could reproduce the problem only when optimization was disabled:

Workaround: Passing --release flag resolves error.

@RalfJung
Copy link
Member

I'm not sure if this really works; My understanding is that normal #[inline] only works if optimization is enabled, but as mentioned in #133035, I could reproduce the problem only when optimization was disabled:

#[inline], with or without optimizations, has the effect of suppressing codegen of the function if it's not actually called. That's what works around the selection bugs here.

At least one of tier 2 target is also affected.

But only with -Zbuild-std. The std we ship seems to be working fine?

@workingjubilee
Copy link
Member

-Zbuild-std is also a nightly feature and it is well-known that the defaults for building the core and compiler builtins need to be different than they actually are set.

@tgross35
Copy link
Contributor

I have a wip for the compiler change at #133050

tgross35 added a commit to tgross35/rust that referenced this pull request Nov 14, 2024
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
tgross35 added a commit to tgross35/rust that referenced this pull request Nov 14, 2024
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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 15, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LLVM ERROR about f128 on AIX and SPARC32 when building core without optimization
6 participants