Block SIMD in transmute_immediate; delete `OperandValueKind` by scottmcm · Pull Request #143410 · rust-lang/rust

scottmcm · 2025-07-04T05:32:58Z

Vectors have been causing me problems for years in this code, for example #110021 (comment) and #143194

See conversation in https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Is.20transmuting.20a.20.60T.60.20to.20.60Tx1.60.20.28one-element.20SIMD.20vector.29.20UB.3F/near/526262799.

By blocking SIMD in transmute_immediate it can be simplified to just take the Scalars involved -- the backend types can be gotten from those Scalars, rather than needing to be passed. And there's an assert added to ICE it if it does get hit.

Accordingly, this changes rvalue_creates_operand to not send SIMD transmutes through the operand path, but to always go through memory instead, like they did back before #108442.

And thanks to those changes, I could also remove the OperandValueKind type that I added back then which @RalfJung rightly considers pretty sketchy.

cc @folkertdev @workingjubilee from the zulip conversation too

See conversation in <https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Is.20transmuting.20a.20.60T.60.20to.20.60Tx1.60.20.28one-element.20SIMD.20vector.29.20UB.3F/near/526262799>.

rustbot · 2025-07-04T05:33:02Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-04T05:33:04Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

scottmcm · 2025-07-04T05:35:09Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

-                    let in_a_ibty = bx.scalar_pair_element_backend_type(operand.layout, 0, false);
-                    let in_b_ibty = bx.scalar_pair_element_backend_type(operand.layout, 1, false);
-                    let out_a_ibty = bx.scalar_pair_element_backend_type(cast, 0, false);
-                    let out_b_ibty = bx.scalar_pair_element_backend_type(cast, 1, false);


These just disappear entirely because transmute_immediate no longer needs them as arguments 🎉

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

RalfJung · 2025-07-04T19:23:52Z

At this point I am happy with the codegen changes, but I have a hard time interpreting the test changes. Wouldn't mind someone else to take a look at those :)

workingjubilee · 2025-07-05T01:12:32Z

The test changes seem to be fine.

@bors r+

bors · 2025-07-05T01:12:35Z

📌 Commit 4e61527 has been approved by workingjubilee

It is now in the queue for this repository.

workingjubilee · 2025-07-05T01:15:57Z

@bors r=RalfJung,workingjubilee

bors · 2025-07-05T01:16:00Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Rollup of 8 pull requests #143459

bors · 2025-07-05T01:16:01Z

📌 Commit 4e61527 has been approved by RalfJung,workingjubilee

It is now in the queue for this repository.

Rollup of 11 pull requests Successful merges: - #142440 (`tests/ui`: A New Order [14/N]) - #143040 (Add `const Rem`) - #143086 (Update poison.rs to fix the typo (sys->sync)) - #143202 (`tests/ui`: A New Order [18/N]) - #143296 (`tests/ui`: A New Order [21/N]) - #143297 (`tests/ui`: A New Order [22/N]) - #143299 (`tests/ui`: A New Order [24/N]) - #143300 (`tests/ui`: A New Order [25/N]) - #143397 (test passing a `VaList` from rust to C) - #143410 (Block SIMD in transmute_immediate; delete `OperandValueKind`) - #143452 (Fix CLI completion check in `tidy`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #143410 - scottmcm:redo-transmute-again, r=RalfJung,workingjubilee Block SIMD in transmute_immediate; delete `OperandValueKind` Vectors have been causing me problems for years in this code, for example #110021 (comment) and #143194 See conversation in <https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Is.20transmuting.20a.20.60T.60.20to.20.60Tx1.60.20.28one-element.20SIMD.20vector.29.20UB.3F/near/526262799>. By blocking SIMD in `transmute_immediate` it can be simplified to just take the `Scalar`s involved -- the backend types can be gotten from those `Scalar`s, rather than needing to be passed. And there's an assert added to ICE it if it does get hit. Accordingly, this changes `rvalue_creates_operand` to not send SIMD transmutes through the operand path, but to always go through memory instead, like they did back before #108442. And thanks to those changes, I could also remove the `OperandValueKind` type that I added back then which `@RalfJung` rightly considers pretty sketchy. cc `@folkertdev` `@workingjubilee` from the zulip conversation too

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s Draft for now because it's built on Ralf's #143291 Inspired by #138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in #143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's #143291~~ Inspired by #138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in #143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's rust-lang/rust#143291~~ Inspired by rust-lang/rust#138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in rust-lang/rust#143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Ralf suggested in <rust-lang#143410 (comment)> that UB transmutes shouldn't be trapping, which happened for the one path that PR was changing, but there's another path as well, so this PR changes that other path to match.

…fleLapkin Make UB transmutes really UB in LLVM Ralf suggested in <rust-lang#143410 (comment)> that UB transmutes shouldn't be trapping, which happened for the one path *that* PR was changing, but there's another path as well, so *this* PR changes that other path to match. r? codegen

Rollup merge of #143718 - scottmcm:ub-transmute-is-ub, r=WaffleLapkin Make UB transmutes really UB in LLVM Ralf suggested in <#143410 (comment)> that UB transmutes shouldn't be trapping, which happened for the one path *that* PR was changing, but there's another path as well, so *this* PR changes that other path to match. r? codegen

Ralf suggested in <rust-lang#143410 (comment)> that UB transmutes shouldn't be trapping, which happened for the one path that PR was changing, but there's another path as well, so this PR changes that other path to match.

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's rust-lang/rust#143291~~ Inspired by rust-lang/rust#138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in rust-lang/rust#143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Make UB transmutes really UB in LLVM Ralf suggested in <rust-lang/rust#143410 (comment)> that UB transmutes shouldn't be trapping, which happened for the one path *that* PR was changing, but there's another path as well, so *this* PR changes that other path to match. r? codegen

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's rust-lang/rust#143291~~ Inspired by rust-lang/rust#138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in rust-lang/rust#143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Make UB transmutes really UB in LLVM Ralf suggested in <rust-lang/rust#143410 (comment)> that UB transmutes shouldn't be trapping, which happened for the one path *that* PR was changing, but there's another path as well, so *this* PR changes that other path to match. r? codegen

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's rust-lang/rust#143291~~ Inspired by rust-lang/rust#138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in rust-lang/rust#143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Block SIMD in transmute_immediate; delete OperandValueKind

5292554

See conversation in <https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Is.20transmuting.20a.20.60T.60.20to.20.60Tx1.60.20.28one-element.20SIMD.20vector.29.20UB.3F/near/526262799>.

rustbot assigned SparrowLii Jul 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 4, 2025

scottmcm commented Jul 4, 2025

View reviewed changes