Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference type support in MachInst backend and on AArch64 #1852

Merged
merged 3 commits into from
Jul 14, 2020

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Jun 9, 2020

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.

@cfallin cfallin added the cranelift:area:aarch64 Issues related to AArch64 backend. label Jun 9, 2020
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm labels Jun 10, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:wasm"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton changed the base branch from master to main June 25, 2020 18:48
@cfallin cfallin changed the title Initial reftype support in aarch64, modulo safepoints. Reference type support in MachInst backend and on AArch64 Jun 27, 2020
@github-actions github-actions bot added cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen labels Jun 27, 2020
@cfallin
Copy link
Member Author

cfallin commented Jun 29, 2020

This is wrong. i128 is supposed to be stored as two i64, just like i64 is supposed to be stored as two i32 on 32bit systems. Storing it in a vector register makes it a lot harder to generate efficient math operations for i128.

Fair enough, removed. It was used internally (produced by and matched by the ABI code) to communicate to store_stack / load_stack that a store/load needs to handle the whole 128-bit vector register, in the case that the regalloc cannot provide a vreg/type associated with a spill. In other words this match arm would not have been triggered by an I128 in the incoming CLIF. But the regalloc should not do this in practice (it only lacks a vreg for a spill/reload when it is a reference from a realreg being spilled to a stackmap slot), so we can just remove the match case. I would expect that when we implement true I128 support, we would lower it to 64-bit ops, and the spill/reload code would not need to reason about I128s.

@cfallin cfallin force-pushed the reftypes branch 2 times, most recently from 927ae63 to 86bb90d Compare July 2, 2020 18:34
@cfallin
Copy link
Member Author

cfallin commented Jul 2, 2020

Rebased to latest and updated to use support in regalloc.rs that was just merged. The PR to do a new release of regalloc is pending, so this depends on the bytecodealliance/regalloc.rs repo and git hash directly, to let CI work; will update this before a any merge.

@julian-seward1, this should be ready for review now!

@cfallin
Copy link
Member Author

cfallin commented Jul 2, 2020

(Actually, spoke too quickly re: regalloc -- we just released 0.0.27 so the dep in this PR can use that now.)

@cfallin cfallin force-pushed the reftypes branch 2 times, most recently from 49e35db to 779056a Compare July 13, 2020 20:03
@cfallin
Copy link
Member Author

cfallin commented Jul 13, 2020

Rebased again after changes to x64 backend.

@julian-seward1 or @bnjbvr, would you mind reviewing this soon before we land additional new-backend PRs, to reduce the rebase overhead somewhat? Thanks!

Copy link
Contributor

@julian-seward1 julian-seward1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks plausible to me (thanks for doing all this). OK to land providing two things are addressed:

  • unexpected types::R32s appearing (as commented below)
  • assure me we're not going to be calling LowerCtx::is_safepoint on every insn.

cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/buffer.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
This commit 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 commit does not include any support for safepoints in aarch64
or the `MachInst` infrastructure; that is in the next commit.

This commit 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).
This commit adds support for generating stackmaps at safepoints to the
new backend framework and to the AArch64 backend in particular. It has
been tested to work with SpiderMonkey.
@cfallin
Copy link
Member Author

cfallin commented Jul 14, 2020

Addressed, thanks!

@cfallin cfallin merged commit fad2aff into bytecodealliance:main Jul 14, 2020
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jul 15, 2020
With bytecodealliance#1852, we now have support for reference types in the new backend
framework generally, and on aarch64 in particular. This PR removes the
test-ignore directive for reftypes tests on this platform.
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jul 15, 2020
With bytecodealliance#1852, we now have support for reference types in the new backend
framework generally, and on aarch64 in particular. This PR removes the
test-ignore directive for reftypes tests on this platform.

Closes bytecodealliance#1886.
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jul 15, 2020
With bytecodealliance#1852, we now have support for reference types in the new backend
framework generally, and on aarch64 in particular. This PR removes the
test-ignore directive for reftypes tests on this platform.

Closes bytecodealliance#1886.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 17, 2020
…e36d0256ed1f6a0e18dc02b3. r=bbouvier

This patch updates the vendored version of Cranelift, pulling in the
reference-types support recently merged in Cranelift's PR
bytecodealliance/wasmtime#1852. Usage of this update to support reftypes
in SpiderMonkey on aarch64 is added in the subsequent commit.

Differential Revision: https://phabricator.services.mozilla.com/D83582
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jul 17, 2020
…e36d0256ed1f6a0e18dc02b3. r=bbouvier

This patch updates the vendored version of Cranelift, pulling in the
reference-types support recently merged in Cranelift's PR
bytecodealliance/wasmtime#1852. Usage of this update to support reftypes
in SpiderMonkey on aarch64 is added in the subsequent commit.

Differential Revision: https://phabricator.services.mozilla.com/D83582
@cfallin cfallin deleted the reftypes branch January 6, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift:wasm cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants