cranelift-interpreter: Fix panic when bitcasting SIMD values

[jan-justin]

Fixes panic caused due to exceeding expected bounds when creating vector values, as mentioned over at #5915

[abrown]

@afonso360, do you want to take a look at this? (@jan-justin, thanks for the contribution!).

[afonso360]

LGTM! Thanks! Just a small note.

[afonso360]

This seems to not handle big endian bitcasts on s390x. Can we add an assert somewhere here to ensure that we panic in that case? Something along these lines:

assert_eq!(
    mem_flags.endianness(Endianness::Little),
    Endianness::Little,
    "Only little endian bitcasts are supported"
);

[jan-justin]

Sure, I could certainly do that.

I would, however, like to know where it seems to indicate that the bitcasts on s390x are not handled? It might be useful for me in future since the CI run is indicating green for s390x. It failed on a previous run when I used DataValue::read_from_slice_ne variant.

[afonso360]

Well, I adapted the testcases into big endian and tried it out in s390x, and they are pointing out different results.

;; cargo run --target=s390x-unknown-linux-gnu  -- test ./filetests/filetests/runtests/simd-bitcast.clif

test interpret
test run
target s390x

function %bitcast_more_lanes_big(i64x2) -> i32x4 {
block0(v0: i64x2):
    v1 = bitcast.i32x4 big v0
    return v1
}
; run: %bitcast_more_lanes_big(0x00000000000000000000000000000000) == 0x00000000000000000000000000000000
; run: %bitcast_more_lanes_big(0x00000000000000ff00000000000000ff) == 0x000000ff00000000000000ff00000000

function %bitcast_fewer_lanes_big(i16x8) -> i64x2 {
block0(v0: i16x8):
    v1 = bitcast.i64x2 big v0
    return v1
}
; run: %bitcast_fewer_lanes_big(0x00000000000000000000000000000000) == 0x00000000000000000000000000000000
; run: %bitcast_fewer_lanes_big(0x00ff0000000000000000000000000000) == 0x00000000000000ff0000000000000000

Those would be good tests to add though! I don't think we have many big endian bitcast runtests. Although it should be noted that only s390x really supports this, I don't think any of the other backends do.

I don't think using ne here is the right solution since we explicitly pick one of the two. I think to make this work with big endian we would have to map each lane, apply a to_be/to_le depending on memflags.

[jan-justin]

I see, thank you for the clarification!

So then is your preference for me to include a panic on big endian bitcasts, or to spend some additional time ensuring that it is correctly handled?

[afonso360]

Up to you! I'm happy either way.

[afonso360]

That test does pass on s390x, and at least at a first glance it makes some sense to me, since the first lane glance it appears that that lane gets mirrored in the i64? This at least seems consistent with the behavior described in page 12 of the Vector Bitcast Presentation which essentially makes bitcast a permute? Sorry if I can't help too much here, this stuff really confuses me.

I've tested a few more cases to try and explore this a bit more (these all pass in the s390x backend today):

; run: %bitcast_fewer_lanes_big(0x00000000000000000000000000000000) == 0x00000000000000000000000000000000
; run: %bitcast_fewer_lanes_big(0x00ff0000000000000000000000000000) == 0x00000000000000ff0000000000000000
; run: %bitcast_fewer_lanes_big(0x00ff00ff000000000000000000000000) == 0x0000000000ff00ff0000000000000000
; run: %bitcast_fewer_lanes_big(0x00ff00ff000000ff0000000000000000) == 0x00ff000000ff00ff0000000000000000
; run: %bitcast_fewer_lanes_big(0x00ff00ff000000ff00000000000000ff) == 0x00ff000000ff00ff00ff000000000000
; run: %bitcast_fewer_lanes_big(0x00ff00ff000000ff00ff0000000000ff) == 0x00ff000000ff00ff00ff0000000000ff

Edit:
Hah! I just got the result you were pointing to by doing something different!

; run: %bitcast_fewer_lanes_big(0x00ff0000000000000000000000000000)                        == 0x00000000000000ff0000000000000000
; run: %bitcast_fewer_lanes_big([0x00ff 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000 0x0000]) == 0x000000000000000000ff000000000000

Both of these tests pass on the s390x backend! Which is something I wasn't expecting. It looks like specifying the lanes explicitly does something different! This is really confusing, but maybe it helps explain why you were getting that result.

I think we may have some endianness inconsistency when dealing with loading values in the testsuite.

[jan-justin]

That is indeed interesting!

I will try to dig a little deeper to see if I can shed some light on this.

Thank you for the additional information, it is really appreciated!

[jan-justin]

Hey,

So I removed the explicit calls to DataValue::read_from_slice since it was superfluous. Nevertheless, it seems the output when running the Big endian tests through the interpreter still does not yield the expected results you showed.

I won't hold the fix hostage any longer than I already have, so I have added a panic when attempting to bitcast on vectors when the big endian mem flag is set.

[afonso360]

I think disabling big endian for now seems like a reasonable solutions. I'm not entirely sure what is up with filetest infrastructure. Thanks for looking into this!

[jan-justin]

No problem at all. Thank you for your patience!