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

aarch64: Initial work to transition backend to ISLE #3541

Merged
merged 6 commits into from Nov 18, 2021

Conversation

alexcrichton
Copy link
Member

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).

;; First up some special cases for constants which match certain custom
;; conditions. Note that these have explicit priorities to explicitly list
;; that we prefer instructions in the order that they're listed here. Otherwise
;; ISLE has no way of determining which is the more relevant matcher.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels Nov 16, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"

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

  • cfallin: isle
  • fitzgen: isle

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

Learn more.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

This looks great to me, at least wrt aarch64 details! (Happy to defer to @fitzgen re: generalizing the framework/bindings etc.) Thanks for filling out the multi-architecture story so quickly here.

.gitattributes Outdated Show resolved Hide resolved
;; First up some special cases for constants which match certain custom
;; conditions. Note that these have explicit priorities to explicitly list
;; that we prefer instructions in the order that they're listed here. Otherwise
;; ISLE has no way of determining which is the more relevant matcher.
Copy link
Member

Choose a reason for hiding this comment

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

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!

cranelift/codegen/src/isa/aarch64/lower/isle.rs Outdated Show resolved Hide resolved
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great! One small suggestion below

cranelift/codegen/src/isa/isle.rs Outdated Show resolved Hide resolved
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).
@alexcrichton alexcrichton merged commit 1141169 into bytecodealliance:main Nov 18, 2021
@alexcrichton alexcrichton deleted the aarch64-isle branch November 18, 2021 16:38
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 Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants