x64: Ensure that constants are always 16 bytes for XmmMem #4790
Conversation
| @@ -0,0 +1,17 @@ | |||
| test run | |||
| set enable_llvm_abi_extensions | |||
| target x86_64 | |||
There was a problem hiding this comment.
Should we just run this on all architectures as well as the interpreter? The bug was x64-specific but the test should work everywhere and it would be nice to know if it didn't.
There was a problem hiding this comment.
As a further suggestion, we can also remove the set enable_llvm_abi_extensions since we no longer have i128's in this new minimized test case.
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| @@ -806,7 +806,7 @@ macro_rules! isle_prelude_methods { | |||
|
|
|||
| #[inline] | |||
| fn emit_u128_le_const(&mut self, value: u128) -> VCodeConstant { | |||
| let data = VCodeConstantData::Generated(value.to_le_bytes().iter().cloned().collect()); | |||
| let data = VCodeConstantData::Generated(value.to_le_bytes().iter().copied().collect()); | |||
There was a problem hiding this comment.
Oh, after Chris' suggestion, now you can simplify this to value.to_le_bytes().into(), I think.
There was a problem hiding this comment.
I guess ConstantData doesn't have a From implementation for arrays, but .as_slice().into() ought to work at least.
There was a problem hiding this comment.
Looks like we're stuck with my verbose monstrosity:
809 | let data = VCodeConstantData::Generated(value.to_le_bytes().iter().into());
| ^^^^ the trait `From<core::slice::Iter<'_, u8>>` is not implemented for `ConstantData`
There was a problem hiding this comment.
The problem there is the .iter().
There was a problem hiding this comment.
Yeah, I get a similar error without it. I had been experimenting :)
809 | let data = VCodeConstantData::Generated(value.to_le_bytes().into());
| ^^^^ the trait `From<[u8; 16]>` is not implemented for `ConstantData`
There was a problem hiding this comment.
I'd figured out that just .into() wouldn't work after I posted my first comment. Did you try my second suggestion, of .as_slice().into()?
There was a problem hiding this comment.
Ah! Totally missed your second comment, that works 👍
There was a problem hiding this comment.
Hooray! It'd probably be nice if ConstantData actually had a From<[T; n]> implementation too, but I didn't want to suggest doing that in this PR. 😅
Ensure that constants generated for the memory case of XmmMem values are always 16 bytes, ensuring that we don't accidantally perform an unaligned load.
Fixes #4761