rustc: Use LLVM's new saturating float-to-int intrinsics #84339

alexcrichton · 2021-04-19T18:01:00Z

This commit updates rustc, with an applicable LLVM version, to use
LLVM's new llvm.fpto{u,s}i.sat.*.* intrinsics to implement saturating
floating-point-to-int conversions. This results in a little bit tighter
codegen for x86/x86_64, but the main purpose of this is to prepare for
upcoming changes to the WebAssembly backend in LLVM where wasm's
saturating float-to-int instructions will now be implemented with these
intrinsics.

This change allows simplifying a good deal of surrounding code, namely
removing a lot of wasm-specific behavior. WebAssembly no longer has any
special-casing of saturating arithmetic instructions and the need for
fptoint_may_trap is gone and all handling code for that is now
removed. This means that the only wasm-specific logic is in the
fpto{s,u}i instructions which only get used for "out of bounds is
undefined behavior". This does mean that for the WebAssembly target
specifically the Rust compiler will no longer be 100% compatible with
pre-LLVM 12 versions, but it seems like that's unlikely to be relied on
by too many folks.

Note that this change does immediately regress the codegen of saturating
float-to-int casts on WebAssembly due to the specialization of the LLVM
intrinsic not being present in our LLVM fork just yet. I'll be following
up with an LLVM update to pull in those patches, but affects a few other
SIMD things in flight for WebAssembly so I wanted to separate this change.

Eventually the entire cast_float_to_int function can be removed when
LLVM 12 is the minimum version, but that will require sinking the
complexity of it into other backends such as Cranelfit.

rust-highfive · 2021-04-19T18:01:03Z

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2021-04-19T18:01:23Z

For comparison, this is the difference on x86_64 of the codegen differences.

alexcrichton · 2021-04-19T18:14:09Z

Also, on second though, I went ahead and removed as many WebAssembly-specific bits here as I could, so I think that this represents the final state of what this will look like in the long run (until pre-LLVM-12 is dropped). The consequences of this though is that WebAssembly targets on pre-LLVM-12 won't work (because the intrinsic won't be defined) and the WebAssembly target with +nontrapping-fptoint will not have as great codegen as before until llvm/llvm-project@5c72975 is cherry-picked to our LLVM fork. I plan to do this soon once some other SIMD related things have landed.

nagisa · 2021-04-20T22:56:02Z

r? @nagisa

I don't believe @matthewjasper been active lately.

nagisa

This broadly speaking seems good to me, but see remarks inline on some sticking points.

r=me once those are resolved.

compiler/rustc_codegen_llvm/src/builder.rs

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

nagisa · 2021-04-20T23:18:08Z

compiler/rustc_codegen_llvm/src/builder.rs

-        // Note that we skip the wasm intrinsics for vector types where `fptoui`
-        // must be used instead.
-        if self.wasm_and_missing_nontrapping_fptoint() {
+        // This intrinsic is only used with non-saturating casts that have UB on


Nit: this is not really an intrinsic, but rather a builder method/instruction builder/etc?

From this comment it is no longer clear why it is important that we specifically go through an effort on wasm to pick a behaviour for what's otherwise an undefined behaviour. It would be good to keep the remark about the intrinsics producing better machine code.

(as a side note: From spelunking it seems that simd_cast also uses this (cc @workingjubilee @calebzulawski) which is potentially undesired (similar to the issue fixed in #84274). This implementation would also crash in a simd setting on wasm, but that's a future improvement)

It does seem a little odd to trap on UB, my understanding is that this function is really only for {f32,f64}::to_int_unchecked which assumes the caller already checked the bounds.

This doesn't crash on wasm simd (we test the simd version of to_int_unchecked on wasm)--it looks like it falls back to the generic llvm instruction which supports vectors

I've changed "intrinsic" to "method", and I've elaborated on the comment to hopefully help explain why wasm is different here.

This should work for SIMD types since it specifically avoids (a few lines below) any input types that are vectors.

bjorn3 · 2021-04-21T07:26:51Z

Eventually the entire cast_float_to_int function can be removed when
LLVM 12 is the minimum version, but that will require sinking the
complexity of it into other backends such as Cranelfit.

Cranelift specifically only has a saturating and trapping version of the float-to-int conversion instruction. There is none that has UB or returns a bogus result.

This commit updates rustc, with an applicable LLVM version, to use LLVM's new `llvm.fpto{u,s}i.sat.*.*` intrinsics to implement saturating floating-point-to-int conversions. This results in a little bit tighter codegen for x86/x86_64, but the main purpose of this is to prepare for upcoming changes to the WebAssembly backend in LLVM where wasm's saturating float-to-int instructions will now be implemented with these intrinsics. This change allows simplifying a good deal of surrounding code, namely removing a lot of wasm-specific behavior. WebAssembly no longer has any special-casing of saturating arithmetic instructions and the need for `fptoint_may_trap` is gone and all handling code for that is now removed. This means that the only wasm-specific logic is in the `fpto{s,u}i` instructions which only get used for "out of bounds is undefined behavior". This does mean that for the WebAssembly target specifically the Rust compiler will no longer be 100% compatible with pre-LLVM 12 versions, but it seems like that's unlikely to be relied on by too many folks. Note that this change does immediately regress the codegen of saturating float-to-int casts on WebAssembly due to the specialization of the LLVM intrinsic not being present in our LLVM fork just yet. I'll be following up with an LLVM update to pull in those patches, but affects a few other SIMD things in flight for WebAssembly so I wanted to separate this change. Eventually the entire `cast_float_to_int` function can be removed when LLVM 12 is the minimum version, but that will require sinking the complexity of it into other backends such as Cranelfit.

alexcrichton · 2021-04-21T14:17:07Z

Great! Then when LLVM 12 is the minimum supported version we can delete even more code and just assume the builder methods do the right thing.

alexcrichton · 2021-04-21T14:17:22Z

@bors: r=nagisa

bors · 2021-04-21T14:17:24Z

📌 Commit de2a460 has been approved by nagisa

bors · 2021-04-21T14:17:25Z

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

…gisa rustc: Use LLVM's new saturating float-to-int intrinsics This commit updates rustc, with an applicable LLVM version, to use LLVM's new `llvm.fpto{u,s}i.sat.*.*` intrinsics to implement saturating floating-point-to-int conversions. This results in a little bit tighter codegen for x86/x86_64, but the main purpose of this is to prepare for upcoming changes to the WebAssembly backend in LLVM where wasm's saturating float-to-int instructions will now be implemented with these intrinsics. This change allows simplifying a good deal of surrounding code, namely removing a lot of wasm-specific behavior. WebAssembly no longer has any special-casing of saturating arithmetic instructions and the need for `fptoint_may_trap` is gone and all handling code for that is now removed. This means that the only wasm-specific logic is in the `fpto{s,u}i` instructions which only get used for "out of bounds is undefined behavior". This does mean that for the WebAssembly target specifically the Rust compiler will no longer be 100% compatible with pre-LLVM 12 versions, but it seems like that's unlikely to be relied on by too many folks. Note that this change does immediately regress the codegen of saturating float-to-int casts on WebAssembly due to the specialization of the LLVM intrinsic not being present in our LLVM fork just yet. I'll be following up with an LLVM update to pull in those patches, but affects a few other SIMD things in flight for WebAssembly so I wanted to separate this change. Eventually the entire `cast_float_to_int` function can be removed when LLVM 12 is the minimum version, but that will require sinking the complexity of it into other backends such as Cranelfit.

ehuss · 2021-04-22T19:29:00Z

@bors r-

This failed on the dist-riscv64-linux job with a seg fault building gimli: #84432 (comment) Confirmed by running that job locally.

nagisa · 2021-04-23T15:11:45Z

@bors r+

bors · 2021-04-23T15:11:46Z

📌 Commit 35ae752 has been approved by nagisa

bors · 2021-04-23T15:13:50Z

⌛ Testing commit 35ae752 with merge 1e747ccec99be0974f47d70abed4d94eafa1d4d9...

bors · 2021-04-23T15:45:48Z

💔 Test failed - checks-actions

alexcrichton · 2021-04-23T16:01:12Z

I updated the assertions in wasm_casts_trapping.rs and renamed the file to wasm_float_to_int_casts.rs. I then deleted wasm_casts_nontrapping.rs since rustc no longer has any special handling of that feature and the test would just be the same.

@bors: r=nagisa

bors · 2021-04-23T16:01:13Z

📌 Commit ed6dd40 has been approved by nagisa

bors · 2021-04-23T18:35:58Z

⌛ Testing commit ed6dd40 with merge 481ba16...

bors · 2021-04-23T21:03:52Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 481ba16 to master...

This fixes the temporary regression introduced in rust-lang#84339 where the wasm target uses `fpto{s,u}i` intrinsics but the codegen for those intrinsics with the `+nontrapping-fptoint` LLVM feature wasn't very good (aka it didn't use the wasm instruction). The fixes brought in here fix that and also implement the second-to-last simd instruction in LLVM.

Update LLVM for more wasm simd updates This fixes the temporary regression introduced in rust-lang#84339 where the wasm target uses `fpto{s,u}i` intrinsics but the codegen for those intrinsics with the `+nontrapping-fptoint` LLVM feature wasn't very good (aka it didn't use the wasm instruction). The fixes brought in here fix that and also implement the second-to-last simd instruction in LLVM.

This fixes the temporary regression introduced in rust-lang#84339 where the wasm target uses `fpto{s,u}i` intrinsics but the codegen for those intrinsics with the `+nontrapping-fptoint` LLVM feature wasn't very good (aka it didn't use the wasm instruction). The fixes brought in here fix that and also implement the second-to-last simd instruction in LLVM.

Update LLVM for more wasm simd updates This fixes the temporary regression introduced in rust-lang#84339 where the wasm target uses `fpto{s,u}i` intrinsics but the codegen for those intrinsics with the `+nontrapping-fptoint` LLVM feature wasn't very good (aka it didn't use the wasm instruction). The fixes brought in here fix that and also implement the second-to-last simd instruction in LLVM.

rust-highfive assigned matthewjasper Apr 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2021

alexcrichton force-pushed the llvm-fptoint-sat branch from a3e784c to f168bfe Compare April 19, 2021 18:12

alexcrichton mentioned this pull request Apr 20, 2021

Penultimate wasm SIMD update (hopefully) rust-lang/llvm-project#101

Merged

rust-highfive assigned nagisa and unassigned matthewjasper Apr 20, 2021

nagisa reviewed Apr 20, 2021

View reviewed changes

alexcrichton force-pushed the llvm-fptoint-sat branch from f168bfe to de2a460 Compare April 21, 2021 14:16

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 Apr 21, 2021

Dylan-DPC-zz mentioned this pull request Apr 22, 2021

Rollup of 12 pull requests #84431

Closed

Dylan-DPC-zz mentioned this pull request Apr 22, 2021

Rollup of 12 pull requests #84432

Closed

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 Apr 22, 2021

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 23, 2021

Update wasm test assertions

ed6dd40

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 Apr 23, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 23, 2021

bors merged commit 481ba16 into rust-lang:master Apr 23, 2021

rustbot added this to the 1.53.0 milestone Apr 23, 2021

alexcrichton deleted the llvm-fptoint-sat branch April 23, 2021 21:08

alexcrichton mentioned this pull request Apr 28, 2021

Update LLVM for more wasm simd updates #84654

Merged

CryZe mentioned this pull request Jul 30, 2021

WASM float to int performance regression since 1.53.0 #87643

Open

daxpedda mentioned this pull request Oct 31, 2023

Stabilize Wasm target features that are in phase 4 and 5 #117457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Use LLVM's new saturating float-to-int intrinsics #84339

rustc: Use LLVM's new saturating float-to-int intrinsics #84339

alexcrichton commented Apr 19, 2021 •

edited

Loading

rust-highfive commented Apr 19, 2021

alexcrichton commented Apr 19, 2021

alexcrichton commented Apr 19, 2021

nagisa commented Apr 20, 2021

nagisa left a comment

nagisa Apr 20, 2021

calebzulawski Apr 21, 2021

alexcrichton Apr 21, 2021

bjorn3 commented Apr 21, 2021

alexcrichton commented Apr 21, 2021

alexcrichton commented Apr 21, 2021

bors commented Apr 21, 2021

bors commented Apr 21, 2021

ehuss commented Apr 22, 2021

nagisa commented Apr 23, 2021

bors commented Apr 23, 2021

bors commented Apr 23, 2021

This comment has been minimized.

bors commented Apr 23, 2021

alexcrichton commented Apr 23, 2021

bors commented Apr 23, 2021

bors commented Apr 23, 2021

bors commented Apr 23, 2021

rustc: Use LLVM's new saturating float-to-int intrinsics #84339

rustc: Use LLVM's new saturating float-to-int intrinsics #84339

Conversation

alexcrichton commented Apr 19, 2021 • edited Loading

rust-highfive commented Apr 19, 2021

alexcrichton commented Apr 19, 2021

alexcrichton commented Apr 19, 2021

nagisa commented Apr 20, 2021

nagisa left a comment

Choose a reason for hiding this comment

nagisa Apr 20, 2021

Choose a reason for hiding this comment

calebzulawski Apr 21, 2021

Choose a reason for hiding this comment

alexcrichton Apr 21, 2021

Choose a reason for hiding this comment

bjorn3 commented Apr 21, 2021

alexcrichton commented Apr 21, 2021

alexcrichton commented Apr 21, 2021

bors commented Apr 21, 2021

bors commented Apr 21, 2021

ehuss commented Apr 22, 2021

nagisa commented Apr 23, 2021

bors commented Apr 23, 2021

bors commented Apr 23, 2021

This comment has been minimized.

bors commented Apr 23, 2021

alexcrichton commented Apr 23, 2021

bors commented Apr 23, 2021

bors commented Apr 23, 2021

bors commented Apr 23, 2021

alexcrichton commented Apr 19, 2021 •

edited

Loading