Wasmtime GitHub notifications bot (Aug 30 2022 at 12:28): Wasmtime GitHub notifications bot (Aug 30 2022 at 12:28):dheaton-arm opened PR #4820 from isle-bitcasts to main:
Implemented support for
bitcaston 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.
-->
Wasmtime GitHub notifications bot (Aug 30 2022 at 14:21):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 30 2022 at 14:21):afonso360 created PR review comment:
I thought there were some issues with this: https://github.com/bytecodealliance/wasmtime/issues/3217 ?
Wasmtime GitHub notifications bot (Aug 30 2022 at 15:33):dheaton-arm submitted PR review.
Wasmtime GitHub notifications bot (Aug 30 2022 at 15:33):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...)
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:34):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:34):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?
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:40):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:40):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:44):dheaton-arm submitted PR review.
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:44):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.
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:56):afonso360 created PR review comment:
Right, as long as that is caught in regalloc we should be safe then!
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:56):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Sep 01 2022 at 09:03):dheaton-arm updated PR #4820 from isle-bitcasts to main.
Wasmtime GitHub notifications bot (Sep 08 2022 at 10:51):dheaton-arm updated PR #4820 from isle-bitcasts to main.
Wasmtime GitHub notifications bot (Sep 20 2022 at 10:37):akirilov-arm requested afonso360 for a review on PR #4820.
Wasmtime GitHub notifications bot (Sep 20 2022 at 10:37):akirilov-arm requested cfallin for a review on PR #4820.
Wasmtime GitHub notifications bot (Sep 20 2022 at 17:08):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 17:08):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 17:08):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
I128case does below -- unless there's a reason I'm missing?
Wasmtime GitHub notifications bot (Sep 21 2022 at 09:52):dheaton-arm updated PR #4820 from isle-bitcasts to main.
Wasmtime GitHub notifications bot (Sep 21 2022 at 10:01):dheaton-arm updated PR #4820 from isle-bitcasts to main.
Wasmtime GitHub notifications bot (Sep 21 2022 at 16:20):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 21 2022 at 16:20):cfallin merged PR #4820.
Last updated: Nov 22 2024 at 17:03 UTC