dheaton-arm opened PR #4820 from isle-bitcasts
to main
:
Implemented support for
bitcast
on vector values for AArch64 and the
interpreter.Also corrected the verifier to ensure that the size, in bits, of the input and
output types match for abitcast
, per the docs.Copyright (c) 2022 Arm Limited
<!--
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.
-->
afonso360 submitted PR review.
afonso360 created PR review comment:
I thought there were some issues with this: https://github.com/bytecodealliance/wasmtime/issues/3217 ?
dheaton-arm submitted PR review.
dheaton-arm created PR review comment:
Hm, I was not aware of that issue. Looking at it, I _think_ this is fine since we're not dealing with precise references to an actual value - merely arbitrary integer values which are treated as references and passed into
is_null
. (Unless, of course, there's something I'm not aware of...)
afonso360 submitted PR review.
afonso360 created PR review comment:
The thing that concerned me the most was a comment by @bjorn3:
I did not attempt to understand why this worked
I am fairly certain it doesn't actually fix the problem, but instead allows a silent miscompilation where the stackmap may be missing entries or have erroneous entries.
In response to someone who tried to do an identical thing (as far as I can tell). So I wonder, do we still have the stackmaps issue on bitcasts? And while this operation is allowed, are we silently allowing bugs?
afonso360 edited PR review comment.
afonso360 edited PR review comment.
dheaton-arm submitted PR review.
dheaton-arm created PR review comment:
So, at least to my reading,
I did not attempt to understand why this worked
referred to them 'solving' the issue by "vendoring regalloc and commenting out the assert to not be there" - which I think is what @bjorn3 means: removing the assert does not fix the problem, merely fails to catch the miscompilation.
I'm hopeful that _that_ issue should not be present here, as I have not modified regalloc in such a way (or at all): if the issue was to show up, I myself would expect to see the initial error message detailed in that issue.
afonso360 created PR review comment:
Right, as long as that is caught in regalloc we should be safe then!
afonso360 submitted PR review.
dheaton-arm updated PR #4820 from isle-bitcasts
to main
.
dheaton-arm updated PR #4820 from isle-bitcasts
to main
.
akirilov-arm requested afonso360 for a review on PR #4820.
akirilov-arm requested cfallin for a review on PR #4820.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Here I think we can return the input directly (it will be implicitly coerced into a register) rather than emit a move instruction, just as the
I128
case does below -- unless there's a reason I'm missing?
dheaton-arm updated PR #4820 from isle-bitcasts
to main
.
dheaton-arm updated PR #4820 from isle-bitcasts
to main
.
cfallin submitted PR review.
cfallin merged PR #4820.
Last updated: Dec 23 2024 at 13:07 UTC