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

Add new MachInst backend and ARM64 support. #1494

Merged
merged 12 commits into from
Apr 16, 2020

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Apr 9, 2020

This PR contributes a new backend framework built around MachInsts (machine instructions) contained in VCode (virtual-registerized code) containers. Furthermore, it contains a working ARM64 backend built in this new framework. The new framework is properly connected into the main Cranelift compilation pipeline, and the appropriate integrations have been made so that wasmtime can run basic Wasm tests correctly on ARM64 hardware.

This patch sequence contains code written by Julian Seward [email protected] and Benjamin Bouvier [email protected], originally developed on a side-branch before rebasing and condensing into this patch series. See the arm64 branch at https://github.com/cfallin/wasmtime for original development history.

This patch also contains code written by Joey Gouly [email protected] and contributed to the above branch. These contributions are "Copyright (c) 2020, Arm Limited."

Closes #1174 and #1075.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Apr 9, 2020
@github-actions
Copy link

github-actions bot commented Apr 9, 2020

Subscribe to Label Action

This issue or pull request has been labeled: "cranelift", "cranelift:meta"

Users Subscribed to "cranelift"

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

Learn more.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Yay! I'll make another pass through this, but here are some initial comments to start with:

crates/runtime/src/traphandlers.rs Outdated Show resolved Hide resolved
tests/custom_signal_handler.rs Outdated Show resolved Hide resolved
crates/jit/src/link.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/Cargo.toml Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/abi.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/abi.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

There are some unnecessary string allocations.

cranelift/codegen/src/isa/arm64/inst/regs.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/arm64/inst/regs.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/arm64/inst/regs.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/arm64/inst/regs.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/arm64/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/dce.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/ir/function.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/postopt.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/verifier/flags.rs Outdated Show resolved Hide resolved
cfallin and others added 9 commits April 11, 2020 17:50
- Add a `simple_legalize()` function that invokes a predetermined set of
  legalizations, without depending on the details of the current
  backend design. This will be used by the new backend pipeline.

- Separate out `has_side_effect()` from the DCE pass. This will be used
  by the new backends' lowering code.

- Add documentation for the `Arm64Call` relocation type.
This removes the old ARM64 backend completely, leaving only an empty
`arm64` module. The tree at this state will not build with the `arm64`
feature enabled, but that feature has to be enabled explicitly (it is
not default). Subsequent patches will fill in the new backend.
This patch adds the MachInst, or Machine Instruction, infrastructure.
This is the machine-independent portion of the new backend design. It
contains the implementation of the "vcode" (virtual-registerized code)
container, the top-level lowering algorithm and compilation pipeline,
and the trait definitions that the machine backends will fill in.

This backend infrastructure is included in the compilation of the
`codegen` crate, but it is not yet tied into the public APIs; that patch
will come last, after all the other pieces are filled in.

This patch contains code written by Julian Seward <[email protected]> and
Benjamin Bouvier <[email protected]>, originally developed on a side-branch
before rebasing and condensing into this patch series. See the `arm64`
branch at `https://github.com/cfallin/wasmtime` for original development
history.

Co-authored-by: Julian Seward <[email protected]>
Co-authored-by: Benjamin Bouvier <[email protected]>
This patch provides the bottom layer of the ARM64 backend: it defines
the `Inst` type, which represents a single machine instruction, and
defines emission routines to produce machine code from a `VCode`
container of `Insts`. The backend cannot produce `Inst`s with just this
patch; that will come with later parts.

This patch contains code written by Julian Seward <[email protected]> and
Benjamin Bouvier <[email protected]>, originally developed on a side-branch
before rebasing and condensing into this patch series. See the `arm64`
branch at `https://github.com/cfallin/wasmtime` for original development
history.

This patch also contains code written by Joey Gouly
<[email protected]> and contributed to the above branch. These
contributions are "Copyright (c) 2020, Arm Limited."

Finally, a contribution from Joey Gouly contains the following notice:

    This is a port of VIXL's Assembler::IsImmLogical.

    Arm has the original copyright on the VIXL code this was ported from
    and is relicensing it under Apache 2 for Cranelift.

Co-authored-by: Julian Seward <[email protected]>
Co-authored-by: Benjamin Bouvier <[email protected]>
Co-authored-by: Joey Gouly <[email protected]>
This patch provides an ARM64 implementation of the ABI-related traits
required by the new backend infrasturcture. It will be used by the
lowering code, when that is in place in a subsequent patch.

This patch contains code written by Julian Seward <[email protected]> and
Benjamin Bouvier <[email protected]>, originally developed on a side-branch
before rebasing and condensing into this patch series. See the `arm64`
branch at `https://github.com/cfallin/wasmtime` for original development
history.

This patch also contains code written by Joey Gouly
<[email protected]> and contributed to the above branch. These
contributions are "Copyright (c) 2020, Arm Limited."

Co-authored-by: Julian Seward <[email protected]>
Co-authored-by: Benjamin Bouvier <[email protected]>
Co-authored-by: Joey Gouly <[email protected]>
This patch adds the lowering implementation that translates Cranelift IR
(CLIF) function bodies to VCode<Inst>, i.e., ARM64 machine instructions.

This patch contains code written by Julian Seward <[email protected]> and
Benjamin Bouvier <[email protected]>, originally developed on a side-branch
before rebasing and condensing into this patch series. See the `arm64`
branch at `https://github.com/cfallin/wasmtime` for original development
history.

This patch also contains code written by Joey Gouly
<[email protected]> and contributed to the above branch. These
contributions are "Copyright (c) 2020, Arm Limited."

Co-authored-by: Julian Seward <[email protected]>
Co-authored-by: Benjamin Bouvier <[email protected]>
Co-authored-by: Joey Gouly <[email protected]>
This patch ties together the previously-committed pieces to implement
the `MachBackend` trait for ARM64.
This patch ties together the new backend infrastructure with the
existing Cranelift codegen APIs.

With all patches in this series up to this patch applied, the ARM64
compiler is now functional and can be used. Two uses of this
functionality -- filecheck-based tests and integration into wasmtime --
will come in subsequent patches.
This commit adds a few odds and ends required to build wasmtime on ARM64
with the new backend. In particular, it adds:

- Support for the `Arm64Call` relocation type.
- Support for fetching the trap PC when a signal is received.
- A hook for `SIGTRAP`, which is sent by the `brk` opcode (in contrast to
  x86's `SIGILL`).

With the patch sequence up to and including this patch applied,
`wasmtime` can now compile and successfully execute code on arm64. Not
all tests pass yet, but basic Wasm/WASI tests work correctly.
@cfallin
Copy link
Member Author

cfallin commented Apr 12, 2020

Thanks @sunfishcode and @bjorn3 for the comments -- I think I've addressed everything, except for the open questions in the unresolved comments above.

cranelift/codegen/src/context.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
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/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

I have done another review pass. I have done all files up to isa/aarch64/inst/regs.rs. I will review the rest another time.

cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

And I am finally done with reviewing for now.

cranelift/codegen/src/isa/aarch64/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/lower.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/lower.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/emit.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/emit.rs Outdated Show resolved Hide resolved
@cfallin
Copy link
Member Author

cfallin commented Apr 14, 2020

Thanks @sunfishcode and @bjorn3 for the further review comments! I think I've addressed everything except for the open questions in unresolved threads above. Happy to take further review comments, of course.

cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/adapter.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/verifier/flags.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/test_utils.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/inst/regs.rs Outdated Show resolved Hide resolved
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

This is very exciting! I think this is ready to land, and we can discuss and iterate from there!

crates/jit/src/link.rs Show resolved Hide resolved
@cfallin
Copy link
Member Author

cfallin commented Apr 15, 2020

Alright, I think I've addressed all comments, finally. Let's wait to see if the latest CI run succeeds; then, @sunfishcode, if you're OK with it, I can hit the Merge button (or feel free to do so yourself if that's the custom here). Thanks for all the review work!

cranelift/codegen/src/machinst/compile.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/mod.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/mod.rs Outdated Show resolved Hide resolved
- Undo temporary changes to default features (`all-arch`) and a
  signal-handler test.
- Remove `SIGTRAP` handler: no longer needed now that we've found an
  "undefined opcode" option on ARM64.
- Rename pp.rs to pretty_print.rs in machinst/.
- Only use empty stack-probe on non-x86. As per a comment in
  rust-lang/compiler-builtins [1], LLVM only supports stack probes on
  x86 and x86-64. Thus, on any other CPU architecture, we cannot refer
  to `__rust_probestack`, because it does not exist.
- Rename arm64 to aarch64.
- Use `target` directive in vcode filetests.
- Run the flags verifier, but without encinfo, when using new backends.
- Clean up warning overrides.
- Fix up use of casts: use u32::from(x) and siblings when possible,
  u32::try_from(x).unwrap() when not, to avoid silent truncation.
- Take immutable `Function` borrows as input; we don't actually
  mutate the input IR.
- Lots of other miscellaneous cleanups.

[1] https://github.com/rust-lang/compiler-builtins/blob/cae3e6ea23739166504f9f9fb50ec070097979d4/src/probestack.rs#L39
@cfallin
Copy link
Member Author

cfallin commented Apr 16, 2020

@sunfishcode actually, I don't have permissions to merge, so please feel do hit the big green button when you're happy!

@cfallin
Copy link
Member Author

cfallin commented Apr 16, 2020

(Actually, I take that back, Till just added me. Let me know if you're happy with the final version and I'll merge.)

@cfallin
Copy link
Member Author

cfallin commented Apr 16, 2020

As per IM with @sunfishcode, merging now!

use crate::isa::fde::RegisterMappingError;
#[cfg(feature = "unwind")]
Copy link
Member

@peterhuene peterhuene Apr 16, 2020

Choose a reason for hiding this comment

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

Unfortunately I just caught this. I believe this accidentally moved the #[cfg(feature = "unwind")].

I have an existing PR that this merged conflicted with, so I'll correct it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yikes, that was likely a rebase failure on my part. Sorry about that. (I suppose I got lucky as unwind is enabled by default.) Thanks for fixing!

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:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build new instruction selector / machine-code emission backend
7 participants