elliottt opened PR #4790 from trevor/xmm-mem-alignment-bug
to main
:
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
<!--Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt has marked PR #4790 as ready for review.
cfallin submitted PR review.
cfallin created PR review comment:
Can we delegate to
put_in_xmm_mem
at this point to share logic (then lift theXmmMem
back toXmmMemImm
)?
cfallin created PR review comment:
We can probably use a
u128
here to avoid the awkward iter-chaining below? Then we can do a zero-extend cast when calling this rather than giving0
asupper
.
cfallin submitted PR review.
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
Absolutely unimportant nit: I like to use
copied
instead ofcloned
when I'm expecting it to be cheap, so the compiler can tell me if I've tried to do it to a type that isn'tCopy
.
jameysharp created PR review comment:
Should this comment say "put_in_reg_mem_imm"?
jameysharp created PR review 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.
elliottt updated PR #4790 from trevor/xmm-mem-alignment-bug
to main
.
elliottt requested cfallin for a review on PR #4790.
elliottt updated PR #4790 from trevor/xmm-mem-alignment-bug
to main
.
elliottt created PR review comment:
Fixed this by delegating to a different function here :)
elliottt submitted PR review.
jameysharp created PR review comment:
Oh, after Chris' suggestion, now you can simplify this to
value.to_le_bytes().into()
, I think.
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
I guess
ConstantData
doesn't have aFrom
implementation for arrays, but.as_slice().into()
ought to work at least.
jameysharp submitted PR review.
jameysharp created PR review comment:
Yeah, delegating to a different function worked out very nicely!
elliottt submitted PR review.
elliottt created PR review 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`
jameysharp created PR review comment:
The problem there is the
.iter()
.
jameysharp submitted PR review.
elliottt submitted PR review.
elliottt created PR review 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`
jameysharp submitted PR review.
jameysharp created PR review 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()
?
elliottt updated PR #4790 from trevor/xmm-mem-alignment-bug
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
Ah! Totally missed your second comment, that works :+1:
cfallin submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
Hooray! It'd probably be nice if
ConstantData
actually had aFrom<[T; n]>
implementation too, but I didn't want to suggest doing that in this PR. :sweat_smile:
elliottt has enabled auto merge for PR #4790.
elliottt merged PR #4790.
afonso360 submitted PR review.
afonso360 created PR review comment:
As a further suggestion, we can also remove the
set enable_llvm_abi_extensions
since we no longer havei128
's in this new minimized test case.
Last updated: Dec 23 2024 at 12:05 UTC