wasmtime / PR #6379 cranelift-interpreter: Fix panic when... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6379 cranelift-interpreter: Fix panic when...

Wasmtime GitHub notifications bot (May 15 2023 at 19:08):

jan-justin opened PR #6379 from jan-justin:cranelift-interpreter-bitcast-simd-panic to bytecodealliance:main:

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

Wasmtime GitHub notifications bot (May 15 2023 at 19:08):

jan-justin requested abrown for a review on PR #6379.

Wasmtime GitHub notifications bot (May 15 2023 at 19:08):

jan-justin requested wasmtime-compiler-reviewers for a review on PR #6379.

Wasmtime GitHub notifications bot (May 16 2023 at 06:24):

jan-justin edited PR #6379:

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

Wasmtime GitHub notifications bot (May 16 2023 at 08:31):

jan-justin updated PR #6379.

Wasmtime GitHub notifications bot (May 16 2023 at 12:56):

afonso360 submitted PR review:

LGTM! Thanks! Just a small note.

Wasmtime GitHub notifications bot (May 16 2023 at 12:56):

afonso360 submitted PR review:

LGTM! Thanks! Just a small note.

Wasmtime GitHub notifications bot (May 16 2023 at 12:56):

afonso360 created PR review comment:

This seems to not handle big endian bitcasts. 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"
);

Wasmtime GitHub notifications bot (May 16 2023 at 12:58):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (May 16 2023 at 13:37):

jan-justin created PR review comment:

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.

Wasmtime GitHub notifications bot (May 16 2023 at 14:10):

afonso360 created PR review comment:

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.

Wasmtime GitHub notifications bot (May 17 2023 at 08:24):

jan-justin created PR review comment:

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?

Wasmtime GitHub notifications bot (May 17 2023 at 09:02):

afonso360 created PR review comment:

Up to you! I'm happy either way.

Wasmtime GitHub notifications bot (May 18 2023 at 17:16):

jan-justin created PR review comment:

I will give it a whirl. I need to set up qemu to test it and get back to you.

Wasmtime GitHub notifications bot (May 24 2023 at 15:18):

jan-justin created PR review comment:

Hey, sorry for the delay but I was unable to setup an s390x environment on my M1 MacBook.

At this point it would seem that I would go the assert route, however, I have a question regarding your comment containing the CLIF. Is your comment stating the assertions you expect to be true, or was that simply the output you received when running on a s390x machine?

I ask because have been wrapping myself around an axle over it.

Wasmtime GitHub notifications bot (May 24 2023 at 16:35):

afonso360 created PR review comment:

I mostly took that test ran it in QEMU s390x and recorded the output. I didn't look at it particularly hard, however I have some faith that the s390x backend would be correct in this regard. If you'd like to look further into this there is a very long thread where we nailed down the bitcast semantics

Wasmtime GitHub notifications bot (May 25 2023 at 19:20):

jan-justin created PR review comment:

Thank you, the thread definitely helped me understand the semantics.

I am subsequently able to get the expected output for bitcast_more_lanes_big, however, I am not quite as successful with bitcast_fewer_lanes_big. If you have the time, could you kindly confirm the expected output for bitcast_fewer_lanes_big since I am getting: Failed test: run: %bitcast_fewer_lanes_big(0x00ff0000000000000000000000000000) == 0x00000000000000ff0000000000000000, actual: 0x000000000000000000ff000000000000

Wasmtime GitHub notifications bot (May 25 2023 at 20:37):

afonso360 created PR review comment:

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:
; 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

Wasmtime GitHub notifications bot (May 25 2023 at 20:37):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (May 25 2023 at 20:42):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (May 25 2023 at 20:43):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (May 25 2023 at 20:43):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (May 25 2023 at 20:44):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (May 26 2023 at 07:36):

jan-justin created PR review comment:

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!

Wasmtime GitHub notifications bot (May 31 2023 at 15:33):

jan-justin updated PR #6379.

Wasmtime GitHub notifications bot (Jun 01 2023 at 13:24):

jan-justin updated PR #6379.

Wasmtime GitHub notifications bot (Jun 01 2023 at 14:18):

jan-justin created PR review comment:

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.

Wasmtime GitHub notifications bot (Jun 01 2023 at 21:11):

afonso360 created PR review comment:

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!

Wasmtime GitHub notifications bot (Jun 02 2023 at 06:02):

jan-justin created PR review comment:

No problem at all. Thank you for your patience!

Wasmtime GitHub notifications bot (Jun 02 2023 at 11:53):

afonso360 merged PR #6379.

Last updated: Nov 22 2024 at 16:03 UTC