Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast. #105583

luqmana · 2022-12-12T03:28:47Z

Fixes #105439.

Also cc @erikdesjardins, looks like another place to cleanup as part of #105545

rustbot · 2022-12-12T03:28:53Z

r? @nagisa

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

src/test/ui/simd/issue-105439.rs

erikdesjardins · 2022-12-13T00:25:39Z

compiler/rustc_codegen_ssa/src/mir/operand.rs

+ let ty = bx.cx().immediate_backend_type(field);
+ if bx.type_kind(ty) == TypeKind::Pointer {
+ *llval = bx.pointercast(*llval, ty);
+ }


This removes the illegal bitcast <4 x i32> to [4 x i32], which is good--but it doesn't replace that illegal conversion with a legal one, so we end up returning a value of type <4 x i32> when extracting a field of type [4 x i32].

The repro example happens to work anyways because the next instruction is a store, and both store <4 x i32> and store [4 x i32] have the same effect. But it won't work if we use that value by value. (This may never happen, because I don't think we ever pass LLVM array types by value, but it's technically incorrect.)

To handle this fully, I think you'd have to special-case this kind of newtype field extraction (self Abi::Vector, field Abi::Aggregate) and roundtrip the vector through memory (alloca, store <4 x i32>, load [4 x i32]).

(I thought there might be endianness concerns, but it seems like that's only a problem for not-a-multiple-of-i8 types like i4)

Good point! Split out the case for newtype of simd array

nagisa · 2023-01-09T11:41:29Z

I had started reviewing it a while before, but clearly I’m not getting back to this anytime soon. I'll keep this tab open still, maybe I’ll find some time.

r? rust-lang/compiler

compiler/rustc_codegen_ssa/src/mir/operand.rs

Noratrieb · 2023-01-09T13:50:40Z

I'm not familiar enough with codegen to review this. r? compiler

oli-obk · 2023-01-09T14:18:20Z

considering how subtle this case is, what's your opinion on adding a codegen test?

erikdesjardins · 2023-01-10T00:42:13Z

(I don't think you're asking me but) A codegen test could be useful, although the proper codegen is just a store/load which isn't too interesting.

Whether or not we add a codegen test, I'd suggest changing the existing test to run-pass, and add some additional code and assertions to make sure our codegen behaves properly at runtime. (e.g. generate an i32x4 with some interesting values, call the function to convert it to [i32; 4], and assert that the array elements are correct)

oli-obk · 2023-01-10T08:28:26Z

@rustbot author

JohnCSimon · 2023-04-30T04:01:28Z

@luqmana Ping from triage: Can you post your status on this PR? This has sat idle for a few months.

… bitcast w/ pointercast.

Remove bitcasts in OperandRef::extract_field; only pointercasts should be needed.

luqmana · 2023-05-05T22:48:12Z

Ok, rebased this and addressed review comments. Also switched the test back to run-pass:

# Fails on latest nightly
$ rustc +nightly -vV
rustc 1.71.0-nightly (74c482104 2023-05-04)
binary: rustc
commit-hash: 74c4821045c68d42bb8b8a7c998bdb5c2a72bd0d
commit-date: 2023-05-04
host: aarch64-apple-darwin
release: 1.71.0-nightly
LLVM version: 16.0.2
$ rustc +nightly -O -Zverify-llvm-ir issue-105439.rs
Invalid bitcast
  %16 = bitcast <4 x i32> %15 to [4 x i32]
LLVM ERROR: Broken module found, compilation aborted!

# Passes with PR
$ rustc +stage1 -O -Zverify-llvm-ir issue-105439.rs
$ ./issue-105439 && echo $?
0

@rustbot ready

oli-obk · 2023-05-06T05:35:22Z

@bors r+

bors · 2023-05-06T05:35:24Z

📌 Commit c7c042a has been approved by oli-obk

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#105583 (Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast.) - rust-lang#110094 (clean up `transmute`s in `core`) - rust-lang#111150 (added TraitAlias to check_item() for missing_docs) - rust-lang#111293 (rustc --explain E0726 - grammar fixing (it's => its + add a `the` where it felt right to do so)) - rust-lang#111300 (Emit while_true lint spanning the entire loop condition) - rust-lang#111301 (Remove calls to `mem::forget` and `mem::replace` in `Option::get_or_insert_with`.) - rust-lang#111303 (update Rust Unstable Book docs for `--extern force`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned nagisa Dec 12, 2022

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 Dec 12, 2022

tmiasko reviewed Dec 12, 2022

View reviewed changes

src/test/ui/simd/issue-105439.rs Outdated Show resolved Hide resolved

erikdesjardins reviewed Dec 13, 2022

View reviewed changes

rustbot assigned Noratrieb and unassigned nagisa Jan 9, 2023

Noratrieb reviewed Jan 9, 2023

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/operand.rs Outdated Show resolved Hide resolved

rustbot assigned oli-obk and unassigned Noratrieb Jan 9, 2023

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2023

luqmana added 7 commits May 5, 2023 14:25

Operand::extract_field: only cast llval if it's a pointer and replace…

48af94c

… bitcast w/ pointercast.

Add test.

75f3faf

Make test more targeted.

af69cc0

Don't bitcast aggregate field.

6a5ee11

Update test location.

2942121

Switch test back to run-pass.

7b1eeda

Address review comments.

c7c042a

Remove bitcasts in OperandRef::extract_field; only pointercasts should be needed.

luqmana force-pushed the bitcast-immediates branch from 9553ece to c7c042a Compare May 5, 2023 22:42

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 5, 2023

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

JohnTitor mentioned this pull request May 7, 2023

Rollup of 7 pull requests #111311

Merged

bors merged commit 5859771 into rust-lang:master May 7, 2023

rustbot added this to the 1.71.0 milestone May 7, 2023

luqmana deleted the bitcast-immediates branch May 7, 2023 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast. #105583

Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast. #105583

luqmana commented Dec 12, 2022

rustbot commented Dec 12, 2022

erikdesjardins Dec 13, 2022

luqmana Dec 16, 2022

nagisa commented Jan 9, 2023

Noratrieb commented Jan 9, 2023

oli-obk commented Jan 9, 2023

erikdesjardins commented Jan 10, 2023

oli-obk commented Jan 10, 2023

JohnCSimon commented Apr 30, 2023

luqmana commented May 5, 2023

oli-obk commented May 6, 2023

bors commented May 6, 2023

Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast. #105583

Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast. #105583

Conversation

luqmana commented Dec 12, 2022

rustbot commented Dec 12, 2022

erikdesjardins Dec 13, 2022

Choose a reason for hiding this comment

luqmana Dec 16, 2022

Choose a reason for hiding this comment

nagisa commented Jan 9, 2023

Noratrieb commented Jan 9, 2023

oli-obk commented Jan 9, 2023

erikdesjardins commented Jan 10, 2023

oli-obk commented Jan 10, 2023

JohnCSimon commented Apr 30, 2023

luqmana commented May 5, 2023

oli-obk commented May 6, 2023

bors commented May 6, 2023