Wasmtime GitHub notifications bot (Nov 04 2020 at 19:56): Wasmtime GitHub notifications bot (Nov 04 2020 at 19:56):cfallin requested julian-seward1 for a review on PR #2363.
Wasmtime GitHub notifications bot (Nov 04 2020 at 19:56):cfallin opened PR #2363 from extend-only-if-abi to main:
There has been some confusion over the meaning of the "sign-extend"
(sext) and "zero-extend" (uext) attributes on parameters and return
values in signatures. According to the three implemented backends, these
attributes indicate that a value narrower than a full register should
always be extended in the way specified. However, they are much more
useful if they mean "extend in this way if the ABI requires extending":
only the ABI backend knows whether or not a particular ABI (e.g., x64
SysV vs. x64 Baldrdash) requires extensions, while only the frontend
(CLIF generator) knows whether or not a value is signed, so the two have
to work in concert.This is the result of some very helpful discussion in #2354 (thanks to
@uweigand for raising the issue and @bjorn3 for helping to reason about
it).This change respects the extension attributes in the above way, rather
than unconditionally extending, to avoid potential performance
degradation as we introduce more extension attributes on signatures.<!--
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 (Nov 05 2020 at 19:54):cfallin updated PR #2363 from extend-only-if-abi to main:
There has been some confusion over the meaning of the "sign-extend"
(sext) and "zero-extend" (uext) attributes on parameters and return
values in signatures. According to the three implemented backends, these
attributes indicate that a value narrower than a full register should
always be extended in the way specified. However, they are much more
useful if they mean "extend in this way if the ABI requires extending":
only the ABI backend knows whether or not a particular ABI (e.g., x64
SysV vs. x64 Baldrdash) requires extensions, while only the frontend
(CLIF generator) knows whether or not a value is signed, so the two have
to work in concert.This is the result of some very helpful discussion in #2354 (thanks to
@uweigand for raising the issue and @bjorn3 for helping to reason about
it).This change respects the extension attributes in the above way, rather
than unconditionally extending, to avoid potential performance
degradation as we introduce more extension attributes on signatures.<!--
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 (Nov 12 2020 at 19:21):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):abrown created PR Review Comment:
Why drop the other assertions? (Not saying we shouldn't, but is this enough to verify that the right extension (or absence) is performed?)
Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):abrown created PR Review Comment:
Just to verify: this becomes a
checkbecause using thebaldrdash_system_vadds some instructions we don't care about in this test?
Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):abrown created PR Review Comment:
I sort of wish we had a similar ABI test for x64... not a blocker for this PR but it might come in handy to rule things out if we're troubleshooting in the future.
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:23):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:23):cfallin created PR Review Comment:
That's actually literally the whole function body generated by Cranelift in this case! The Baldrdash ABI generates "naked" functions, without prologues or epilogues, because SpiderMonkey expects to generate those itself.
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:24):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:24):cfallin created PR Review Comment:
More precisely, because it's the only instruction :-)
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:25):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:25):cfallin created PR Review Comment:
For sure, I agree -- I'll try to do this if/when I touch the ABI code next.
Wasmtime GitHub notifications bot (Nov 12 2020 at 20:26):cfallin merged PR #2363.
Last updated: Nov 22 2024 at 16:03 UTC