Stream: git-wasmtime

Topic: wasmtime / PR #2363 Do value-extensions at ABI boundaries...


view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2020 at 19:56):

cfallin requested julian-seward1 for a review on PR #2363.

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):

abrown submitted PR Review.

view this post on Zulip 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?)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 19:21):

abrown created PR Review Comment:

Just to verify: this becomes a check because using the baldrdash_system_v adds some instructions we don't care about in this test?

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 20:23):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 20:24):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 20:24):

cfallin created PR Review Comment:

More precisely, because it's the only instruction :-)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 20:25):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 12 2020 at 20:26):

cfallin merged PR #2363.


Last updated: Dec 23 2024 at 12:05 UTC