Stream: git-wasmtime

Topic: wasmtime / PR #1852 Reference type support in MachInst ba...

Wasmtime GitHub notifications bot (Jun 27 2020 at 01:04):

cfallin edited PR #1852 from reftypes to main:

This PR adds the inital support to allow reftypes to flow through the
program when targetting aarch64. It also adds a fix to the
ModuleTranslationState needed to send R32/R64 types over from the
SpiderMonkey embedding.

This PR does not include any support for safepoints in aarch64 or the
MachInst infrastructure. That work will come as a second PR, following
some needed changes to regalloc.rs.

This PR also makes a drive-by improvement to Bint, avoiding an
unneeded zero-extension op when the extended value comes directly from a
conditional-set (which produces a full-width 0 or 1).

With this PR, and a companion patch in SpiderMonkey, we're able to make it
through all of the reftypes tests in the spec testsuite, though we aren't emitting
stackmaps yet so if the GC had been triggered we would have been in trouble.
Still, it's a start!

<!--

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 (Jun 27 2020 at 01:08):

cfallin edited PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with wasmtime/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jun 27 2020 at 01:09):

cfallin edited PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jun 27 2020 at 01:13):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jun 29 2020 at 23:12):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 02 2020 at 18:33):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 02 2020 at 18:34):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 02 2020 at 18:40):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 13 2020 at 20:01):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 13 2020 at 20:03):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 14 2020 at 15:48):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

nit: "is right here" is hard to parse. Does it mean "is correct here"? Or "at this point in the frame construction, nominal SP = actual SP" etc? Can you clarify?

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

Errr, *end* is too vague. Please specify exactly. Do you mean: the code offset for the last (highest-addressed) byte of the instruction ..

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

Can it happen that Some(t) is inconsistent with the second (here, ignored) component of the tuple? It would be good to add a comment explaining the meaning of this function. In particular why the first component has priority.

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

nit: ..rc feels a bit too terse here; can you call it ref_type_regclass ?

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

How is this used? In particular, will we have to call is_safepoint on every insn that is processed? That would seem to be an undesirable compiler performance hit, if so? (Tell me it ain't so!)

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

nano-nit: since most of our users will be coming at this on 64-bit targets, maybe put that first in the disjunction? That way we can save perhaps 1/3 of a nanosecond per call :-)

Wasmtime GitHub notifications bot (Jul 14 2020 at 15:48):

julian-seward1 created PR Review Comment:

Here and below, it concerns me that we are accepting R32 on a 64-bit target without comment. IIUC, if anything claiming to be R32 turns up on a 64-bit target, we have gone way off the rails -- perhaps even to the extent of having a security problem. In such a circumstance I would much prefer that we bring the system to a halt, even in release builds (== panic in every such match on tys); and similarly for x64 stubs, if you have similarly modified them.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:23):

cfallin updated PR #1852 from reftypes to main:

This PR adds support for reference types in the MachInst backend framework and on AArch64. It comes in two parts: the initial support for allowing R32/R64 types to flow through the Wasm program and call boundaries, and the implementation of opcodes that support them (is_null, etc.); and then stackmap support to allow GCs to occur while Wasm stackframes are active.

This has been tested with SpiderMonkey and is known to work there with GCs occuring during calls from Wasm into JS. We plan to do further testing in regalloc.rs directly (with our checker) too.

This requires a regalloc.rs version bump (with bytecodealliance/regalloc.rs#79 included) and dep version update; the CI build here will be red until that change is made.

<!--

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 (Jul 14 2020 at 17:23):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:23):

cfallin created PR Review Comment:

Done, excellent point.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:23):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:23):

cfallin created PR Review Comment:

Sorry, this was left over from an earlier iteration that did indeed call is_safepoint on every instruction; but we no longer do that, and the function was dead, so I removed it.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:24):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:24):

cfallin created PR Review Comment:

Updated comment, thanks!

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:26):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jul 14 2020 at 17:26):

cfallin created PR Review Comment:

The type should always be consistent with the register class; the Option<Type> is only missing when the regalloc requests a spill of a realreg (for which it doesn't have a vreg, hence no vcode-level type) at a safepoint, and this will always be an I64 reg. Added comments to clarify.

Wasmtime GitHub notifications bot (Jul 14 2020 at 18:22):

cfallin merged PR #1852.

Last updated: Nov 22 2024 at 17:03 UTC