Stream: git-wasmtime

Topic: wasmtime / PR #3541 aarch64: Initial work to transition b...

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:31):

alexcrichton opened PR #3541 from aarch64-isle to main:

This commit is what is hoped to be the initial commit towards migrating
the aarch64 backend to ISLE. There's seemingly a lot of changes here but
it's intended to largely be code motion. The current thinking is to
closely follow the x64 backend for how all this is handled and
organized.

Major changes in this PR are:

• The Inst enum is now defined in ISLE. This avoids having to define
  it in two places (once in Rust and once in ISLE). I've preserved all
  the comments in the ISLE and otherwise this isn't actually a
  functional change from the Rust perspective, it's still the same enum
  according to Rust.

• Lots of little enums and things were moved to ISLE as well. As with
  Inst their definitions didn't change, only where they're defined.
  This will give future ISLE PRs access to all these operations.

• Initial code for lowering iconst, null, and bconst are
  implemented. Ironically none of this is actually used right now
  because constant lowering is handled in put_input_in_regs which
  specially handles constants. Nonetheless I wanted to get at least
  something simple working which shows off how to special case various
  things that are specific to AArch64. In a future PR I plan to hook up
  const-lowering in ISLE to this path so even though
  iconst-the-clif-instruction is never lowered this should use the
  const lowering defined in ISLE rather than elsewhere in the backend
  (eventually leading to the deletion of the non-ISLE lowering).

• The IsleContext skeleton is created and set up for future additions.

• Some code for ISLE that's shared across all backends now lives in
  isle_prelude_methods!() and is deduplicated between the AArch64
  backend and the x64 backend.

• Register mapping is tweaked to do the same thing for AArch64 that it
  does for x64. Namely mapping virtual registers is supported instead of
  just virtual to machine registers.

My main goal with this PR was to get AArch64 into a place where new
instructions can be added with relative ease. Additionally I'm hoping to
figure out as part of this change how much to share for ISLE between
AArch64 and x64 (and other backends).

<!--

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 16 2021 at 23:33):

alexcrichton created PR review comment:

Someone more familiar with AArch64 may want to confirm this, I'm respecting the original ordering that's implemented today, but I'm not sure if it matters matching it precisely or if any of the three one-instruction forms is fine to materialize a constant with.

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:33):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:59):

cfallin created PR review comment:

Some of these type-defs seem like they could be shared with x64 than pub use common::* into this module, probably?

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:59):

cfallin created PR review comment:

Does the syntax support a wildcard (.../isa/*/lower/isle/generated_code.rs) here?

Wasmtime GitHub notifications bot (Nov 16 2021 at 23:59):

cfallin created PR review comment:

Since they're all 1-instruction lowerings, it should be fine (and equal cost) to use any that fit, I think. (Though at least MOVZ and MOVN should be completely mutually exclusive, since all the other bits except the shifted 16-bit field are all 0 or all 1 respectively, but ORI's range overlaps with MOVZ.)

So I think it's slightly better to exclude the priorities here so the decision trie build has more freedom to merge and reorder.

Excellent attention to preserving original behavior though, thanks for taking care with that!

Wasmtime GitHub notifications bot (Nov 17 2021 at 15:47):

alexcrichton updated PR #3541 from aarch64-isle to main.

Wasmtime GitHub notifications bot (Nov 17 2021 at 15:52):

alexcrichton updated PR #3541 from aarch64-isle to main.

Wasmtime GitHub notifications bot (Nov 17 2021 at 15:53):

alexcrichton requested fitzgen for a review on PR #3541.

Wasmtime GitHub notifications bot (Nov 17 2021 at 22:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2021 at 22:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2021 at 22:29):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2021 at 22:29):

fitzgen created PR review comment:

I think this file should be at cranelift/codegen/src/machinst/isle.rs

Wasmtime GitHub notifications bot (Nov 18 2021 at 15:37):

alexcrichton updated PR #3541 from aarch64-isle to main.

Wasmtime GitHub notifications bot (Nov 18 2021 at 15:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 15:38):

alexcrichton created PR review comment:

Oops I thought I did that already...

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:38):

alexcrichton merged PR #3541.

Last updated: Dec 23 2024 at 12:05 UTC