-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
x86-32 float return for 'Rust' ABI: treat all float types consistently #131871
Conversation
f4a2193
to
7b06c64
Compare
Cc @tgross35 |
Huh. I hadn't noticed a bunch of target-specific code has escaped That will be fixed. |
@RalfJung just to confirm: This winds up applying to both i586 and i686, yes? |
I assume so. I didn't change that part of the logic. |
7b06c64
to
a38873d
Compare
Yeah, rustc_target does the non-Rust-ABI handling, but at some point we had to start making Rust ABI handling target-specific... |
a38873d
to
a9f6fd1
Compare
👍 for making this change |
Abi::Scalar(s) => matches!(s.primitive(), Float(_)), | ||
Abi::ScalarPair(s1, s2) => { | ||
matches!(s1.primitive(), Float(_)) || matches!(s2.primitive(), Float(_)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abi::Scalar(s) => matches!(s.primitive(), Float(_)), | |
Abi::ScalarPair(s1, s2) => { | |
matches!(s1.primitive(), Float(_)) || matches!(s2.primitive(), Float(_)) | |
Abi::Scalar(s) => matches!(s.primitive(), Float(F16 | F32 | F64)), | |
Abi::ScalarPair(s1, s2) => { | |
matches!(s1.primitive(), Float(F16 | F32 | F64)) || matches!(s2.primitive(), Float(F16 | F32 | F64)) |
nit: There's no need to include f128
here as the 32-bit x86 ABI already guarantees it gets passed in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should treat all float types uniformly here, that's more future-proof. The result is the same both ways but my code makes it more clear what happens, relying on fewer external assumptions.
We don't care about being consistent with the C ABI here, after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I kinda agree morally with beetrees but not actually: this seems fine as-is.
I don't think it's worth spending too much time on this. While I do need to be able to read this code to refactor it, I do intend to bulldoze it.
And since this seems fine-ish, assuming CI doesn't find a reason to take issue with it, and I would hate to make RalfJung rebase this a bunch: @bors r+ |
…bilee x86-32 float return for 'Rust' ABI: treat all float types consistently This helps with rust-lang#131819: for our own ABI on x86-32, we want to *never* use the float registers. The previous logic only considered F32 and F64, but skipped F16 and F128. So I made the logic just apply to all float types.
…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#127462 (std: uefi: Add basic Env variables) - rust-lang#131537 (Fix range misleading field access) - rust-lang#131838 (bootstrap: allow setting `--jobs` in config.toml) - rust-lang#131871 (x86-32 float return for 'Rust' ABI: treat all float types consistently) - rust-lang#131876 (compiler: Use LLVM's Comdat support) - rust-lang#131890 (Update `use` keyword docs to describe precise capturing) - rust-lang#131899 (Mark unexpected variant res suggestion as having placeholders) - rust-lang#131908 (rustdoc: Switch from FxHash to sha256 for static file hashing.) r? `@ghost` `@rustbot` modify labels: rollup
…bilee x86-32 float return for 'Rust' ABI: treat all float types consistently This helps with rust-lang#131819: for our own ABI on x86-32, we want to *never* use the float registers. The previous logic only considered F32 and F64, but skipped F16 and F128. So I made the logic just apply to all float types.
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#127462 (std: uefi: Add basic Env variables) - rust-lang#131537 (Fix range misleading field access) - rust-lang#131838 (bootstrap: allow setting `--jobs` in config.toml) - rust-lang#131871 (x86-32 float return for 'Rust' ABI: treat all float types consistently) - rust-lang#131876 (compiler: Use LLVM's Comdat support) - rust-lang#131890 (Update `use` keyword docs to describe precise capturing) - rust-lang#131899 (Mark unexpected variant res suggestion as having placeholders) - rust-lang#131908 (rustdoc: Switch from FxHash to sha256 for static file hashing.) - rust-lang#131916 (small interpreter error cleanup) - rust-lang#131919 (zero-sized accesses are fine on null pointers) r? `@ghost` `@rustbot` modify labels: rollup
…bilee x86-32 float return for 'Rust' ABI: treat all float types consistently This helps with rust-lang#131819: for our own ABI on x86-32, we want to *never* use the float registers. The previous logic only considered F32 and F64, but skipped F16 and F128. So I made the logic just apply to all float types.
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#127462 (std: uefi: Add basic Env variables) - rust-lang#131537 (Fix range misleading field access) - rust-lang#131838 (bootstrap: allow setting `--jobs` in config.toml) - rust-lang#131871 (x86-32 float return for 'Rust' ABI: treat all float types consistently) - rust-lang#131890 (Update `use` keyword docs to describe precise capturing) - rust-lang#131899 (Mark unexpected variant res suggestion as having placeholders) - rust-lang#131908 (rustdoc: Switch from FxHash to sha256 for static file hashing.) - rust-lang#131916 (small interpreter error cleanup) - rust-lang#131919 (zero-sized accesses are fine on null pointers) r? `@ghost` `@rustbot` modify labels: rollup
@bors r- |
x86-32 float return for 'Rust' ABI: treat all float types consistently This helps with rust-lang#131819: for our own ABI on x86-32, we want to *never* use the float registers. The previous logic only considered F32 and F64, but skipped F16 and F128. So I made the logic just apply to all float types. try-job: i686-gnu try-job: i686-gnu-nopt
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
I guess CI couldn't find anything wrong this time so it just made up a failure @bors retry |
x86-32 float return for 'Rust' ABI: treat all float types consistently This helps with rust-lang#131819: for our own ABI on x86-32, we want to *never* use the float registers. The previous logic only considered F32 and F64, but skipped F16 and F128. So I made the logic just apply to all float types. try-job: i686-gnu try-job: i686-gnu-nopt
real "make up a PR to get mad at it" hours |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Looks like CI hasn't played enough, still. @bors retry |
@SpriteOvO: 🔑 Insufficient privileges: not in try users |
@bors retry npm error network Invalid response body while trying to fetch |
☀️ Test successful - checks-actions |
Finished benchmarking commit (8bf64f1): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 4.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 780.615s -> 779.598s (-0.13%) |
This helps with #131819: for our own ABI on x86-32, we want to never use the float registers. The previous logic only considered F32 and F64, but skipped F16 and F128. So I made the logic just apply to all float types.
try-job: i686-gnu
try-job: i686-gnu-nopt