Skip to content

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

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

Merged
merged 2 commits into from
Apr 10, 2023

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Apr 6, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented Apr 6, 2023

r? @petrochenkov

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

@rustbot 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
@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member Author

scottmcm commented Apr 6, 2023

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

@scottmcm 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
@petrochenkov
Copy link
Contributor

r? @compiler-errors

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) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

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

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 9, 2023

📌 Commit 621417bf7d51f9345d4e0669a150189e50a231db has been approved by compiler-errors

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 Apr 9, 2023
@bors
Copy link
Collaborator

bors commented Apr 9, 2023

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

@bors
Copy link
Collaborator

bors commented Apr 9, 2023

💔 Test failed - checks-actions

@bors 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
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

r=me

@scottmcm
Copy link
Member Author

scottmcm commented Apr 9, 2023

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

@bors r=compiler-errors

@bors
Copy link
Collaborator

bors commented Apr 9, 2023

📌 Commit d757c4b has been approved by compiler-errors

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 Apr 9, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2023
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
@bors bors merged commit b872552 into rust-lang:master Apr 10, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 10, 2023
@scottmcm scottmcm deleted the fix-110005 branch April 11, 2023 01:46
Noratrieb added a commit to Noratrieb/rust that referenced this pull request May 30, 2023
…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`
Noratrieb added a commit to Noratrieb/rust that referenced this pull request May 30, 2023
…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``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 31, 2023
…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```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 1, 2023
…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`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jul 5, 2025
…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
rust-timer added a commit that referenced this pull request Jul 5, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants