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

[luqmana]

Fixes #105439.

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

[rustbot]

r? @nagisa

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

[erikdesjardins]

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)

[luqmana]

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

[nagisa]

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

[Noratrieb]

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

[oli-obk]

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

[erikdesjardins]

(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]

@rustbot author

[JohnCSimon]

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

[luqmana]

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]

@bors r+

[bors]

📌 Commit c7c042a has been approved by oli-obk

It is now in the queue for this repository.