Add x86 legalization for sshr.i64x2 #1649
Conversation
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @bnjbvr DetailsThis issue or pull request has been labeled: "cranelift", "cranelift:docs", "cranelift:meta", "cranelift:wasm"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| v2 = sshr v0, v1 | ||
| return v2 | ||
| } | ||
| ; run: %ishl_i64x2([1 -1], 0) == [1 -1] |
There was a problem hiding this comment.
I am confused by the use of ishl_64x2 at this point. Shouldn't that be ishr_64x2? The actual results shown are consistent with a signed shift right, not a shift left.
There was a problem hiding this comment.
Also I'd be happier if there were a couple more tests here. Could you add a test that 1000-(63)-000 >>signed 63 produces 111-(64)-111 and that 0111-(63)-111 >>signed 63 produces 000-(64)-000 ?
There was a problem hiding this comment.
Oh, good catch; yeah, that's likely an oversight from copying other tests. I'll fix these and add the additional tests.
julian-seward1
left a comment
julian-seward1
left a comment
There was a problem hiding this comment.
r+ but with the request that the tests are clarified and slightly enhanced, as described.
Previously, the logic was wrong on two counts: - It used the bits of the entire vector (e.g. i32x4 -> 128) instead of just the lane bits (e.g. i32x4 -> 32). - It used the type of the first operand before it was bitcast to its correct type. Remember that, by default, vectors are handed around as i8x16 and we must bitcast them to their correct type for Cranelift's verifier; see bytecodealliance#1147 for discussion on this. This fix simply uses the type of the instruction itself, which is equivalent and hopefully less fragile to any changes.
2 participants
This adds a legalization for
sshr.i64x2and fixes some issues I found while doing this. It is based on the changes from #1645 since some of the display changes there made these issues easier to troubleshoot.