Fix a couple ICEs in the new `CastKind::Transmute` code #110021

scottmcm · 2023-04-06T20:55:45Z

Check the sizes of the immediates, rather than the overall types, when deciding whether we can convert types without going through memory.

Fixes #110005
Fixes #109992
Fixes #110032
cc @matthiaskrgr

Fixes rust-lang#110005

rustbot · 2023-04-06T20:55:50Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

scottmcm · 2023-04-06T22:40:00Z

Oh, convenient, this accidentally made a repro of #109992 too 🙃

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

petrochenkov · 2023-04-07T11:07:18Z

r? @compiler-errors

WaffleLapkin · 2023-04-07T19:21:26Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

+                            Some(OperandRef::new_zst(bx, cast).val)
+                        }
+                        (ScalarOrZst::Scalar(in_scalar), ScalarOrZst::Scalar(out_scalar))
+                            if in_scalar.size(self.cx) == out_scalar.size(self.cx) =>


Why is this check necessary?

For pairs you need to check the inner size because matching size of the whole pair doesn't necessarily imply matching sizes of elements... But surely the operand.layout.size != cast.size check is sufficient for scalars? 🤔

Oh, good call. Let me give that a shot -- might allow deleting more code too...

Removed this check, but ScalarOrZst::size is still needed elsewhere, so I couldn't remove more than that.

Oh, no, vectors. Without this check <4 x i64> to <8 x f32> trips the later debug-assert.

I suppose I could hypothetically bitcast that anyway, but I'm not doing that this PR.

Added some codegen tests for vectors so that when future me breaks this while running --keep-stage-std again, I'll notice without CI 😬

tests/codegen/intrinsics/transmute.rs

compiler-errors · 2023-04-09T05:21:46Z

@bors r+

bors · 2023-04-09T05:21:48Z

📌 Commit 621417bf7d51f9345d4e0669a150189e50a231db has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-04-09T10:30:22Z

⌛ Testing commit 621417bf7d51f9345d4e0669a150189e50a231db with merge 7deb56e0f1c0ca611132e8f7c025896c2398291e...

bors · 2023-04-09T10:53:57Z

💔 Test failed - checks-actions

compiler-errors · 2023-04-09T18:19:09Z

r=me

scottmcm · 2023-04-09T21:55:24Z

I'd forgotten I made that test file only-64bit instead of my usual only-x86_64 🤦

@bors r=compiler-errors

bors · 2023-04-09T21:55:26Z

📌 Commit d757c4b has been approved by compiler-errors

It is now in the queue for this repository.

Rollup of 6 pull requests Successful merges: - rust-lang#109724 (prioritize param env candidates if they don't guide type inference) - rust-lang#110021 (Fix a couple ICEs in the new `CastKind::Transmute` code) - rust-lang#110044 (Avoid some manual slice length calculation) - rust-lang#110115 (compiletest: Use remap-path-prefix only in CI) - rust-lang#110121 (Fix `x check --stage 1` when download-rustc is enabled) - rust-lang#110124 (Some clippy fixes in the compiler) Failed merges: - rust-lang#109752 (Stall auto trait assembly in new solver for int/float vars) r? `@ghost` `@rustbot` modify labels: rollup

…ompiler-errors Add a distinct `OperandValue::ZeroSized` variant for ZSTs These tend to have special handling in a bunch of places anyway, so the variant helps remember that. And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992). As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s. Inspired by rust-lang#110021 (comment), so r? `@compiler-errors`

…ompiler-errors Add a distinct `OperandValue::ZeroSized` variant for ZSTs These tend to have special handling in a bunch of places anyway, so the variant helps remember that. And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992). As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s. Inspired by rust-lang#110021 (comment), so r? ``@compiler-errors``

…ompiler-errors Add a distinct `OperandValue::ZeroSized` variant for ZSTs These tend to have special handling in a bunch of places anyway, so the variant helps remember that. And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992). As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s. Inspired by rust-lang#110021 (comment), so r? ```@compiler-errors```

…ompiler-errors Add a distinct `OperandValue::ZeroSized` variant for ZSTs These tend to have special handling in a bunch of places anyway, so the variant helps remember that. And I think it's easier to grok than `Aggregate`s sometimes being `Immediates` (after all, I previously got that wrong and caused rust-lang#109992). As a minor bonus, it means we don't need to generate poison LLVM values for ZSTs to pass around in `OperandValue::Immediate`s. Inspired by rust-lang#110021 (comment), so r? `@compiler-errors`

…alfJung,workingjubilee Block SIMD in transmute_immediate; delete `OperandValueKind` Vectors have been causing me problems for years in this code, for example rust-lang#110021 (comment) and rust-lang#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 rust-lang#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

Check CastKind::Transmute sizes in a better way

454bca5

Fixes rust-lang#110005

rustbot assigned petrochenkov Apr 6, 2023

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 Apr 6, 2023

This comment has been minimized.

Sign in to view

scottmcm changed the title ~~Check CastKind::Transmute sizes in a better way~~ Fix a couple ICEs in the new CastKind::Transmute code Apr 6, 2023

scottmcm force-pushed the fix-110005 branch from 284ed3a to afba18d Compare April 7, 2023 00:30

compiler-errors reviewed Apr 7, 2023

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/rvalue.rs Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/mir/rvalue.rs Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/mir/rvalue.rs Show resolved Hide resolved

scottmcm mentioned this pull request Apr 7, 2023

[ICE] Couldn't translate Aggregate { sized: true } as backend immediate #110032

Closed

rustbot assigned compiler-errors and unassigned petrochenkov Apr 7, 2023

petrochenkov mentioned this pull request Apr 7, 2023

Erase regions when confirming transmutability candidate #110038

Merged

WaffleLapkin reviewed Apr 7, 2023

View reviewed changes

scottmcm force-pushed the fix-110005 branch 2 times, most recently from 565e3db to 621417b Compare April 7, 2023 21:22

compiler-errors approved these changes Apr 9, 2023

View reviewed changes

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 9, 2023

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 9, 2023

This comment has been minimized.

Sign in to view

Handle not all immediates having abi::Scalars

d757c4b

scottmcm force-pushed the fix-110005 branch from 621417b to d757c4b Compare April 9, 2023 18:17

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 9, 2023

Dylan-DPC mentioned this pull request Apr 10, 2023

Rollup of 6 pull requests #110137

Merged

bors merged commit b872552 into rust-lang:master Apr 10, 2023

rustbot added this to the 1.70.0 milestone Apr 10, 2023

scottmcm deleted the fix-110005 branch April 11, 2023 01:46

scottmcm mentioned this pull request May 7, 2023

Add a distinct OperandValue::ZeroSized variant for ZSTs #111318

Merged

scottmcm mentioned this pull request Jul 4, 2025

Block SIMD in transmute_immediate; delete OperandValueKind #143410

Open

Fix a couple ICEs in the new CastKind::Transmute code #110021

Fix a couple ICEs in the new CastKind::Transmute code #110021

Uh oh!

Conversation

scottmcm commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 6, 2023

Uh oh!

This comment has been minimized.

scottmcm commented Apr 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Apr 7, 2023

Uh oh!

WaffleLapkin Apr 7, 2023

Choose a reason for hiding this comment

Uh oh!

scottmcm Apr 7, 2023

Choose a reason for hiding this comment

Uh oh!

scottmcm Apr 7, 2023

Choose a reason for hiding this comment

Uh oh!

scottmcm Apr 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

compiler-errors commented Apr 9, 2023

Uh oh!

bors commented Apr 9, 2023

Uh oh!

bors commented Apr 9, 2023

Uh oh!

bors commented Apr 9, 2023

Uh oh!

This comment has been minimized.

compiler-errors commented Apr 9, 2023

Uh oh!

scottmcm commented Apr 9, 2023

Uh oh!

bors commented Apr 9, 2023

Uh oh!

Uh oh!

Fix a couple ICEs in the new `CastKind::Transmute` code #110021

Fix a couple ICEs in the new `CastKind::Transmute` code #110021

scottmcm commented Apr 6, 2023 •

edited

Loading