Stream: git-wasmtime

Topic: wasmtime / PR #3545 aarch64: Migrate `iadd` and `isub` to...

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

alexcrichton opened PR #3545 from arm64-iadd-isub-isle to main:

This commit is the first "meaty" instruction added to ISLE for the
AArch64 backend. I chose to pick the first two in the current lowering's
match statement, isub and iadd. These two turned out to be
particularly interesting for a few reasons:

• Both had clearly migratable-to-ISLE behavior along the lines of
  special-casing per type. For example 128-bit and vector arithmetic
  were both easily translateable.

• The iadd instruction has special cases for fusing with a
  multiplication to generate madd which is expressed pretty easily in
  ISLE.

• Otherwise both instructions had a number of forms where they attempted
  to interpret the RHS as various forms of constants, extends, or
  shifts. There's a bit of a design space of how best to represent this
  in ISLE and what I settled on was to have a special case for each form
  of instruction, and the special cases are somewhat duplicated between
  iadd and isub. There's custom "extractors" for the special cases
  and instructions that support these special cases will have an
  rule-per-case.

Overall I think the ISLE transitioned pretty well. I don't think that
the aarch64 backend is going to follow the x64 backend super closely,
though. For example the x64 backend is having a helper-per-instruction
at the moment but with AArch64 it seems to make more sense to only have
a helper-per-enum-variant-of-MInst. This is because the same
instruction (e.g. ALUOp::Sub32) can be expressed with multiple
different forms depending on the payload.

It's worth noting that the ISLE looks like it's a good deal larger than
the code actually being removed from lowering as part of this commit. I
think this is deceptive though because a lot of the logic in
put_input_in_rse_imm12_maybe_negated and alu_inst_imm12 is being
inlined into the ISLE definitions for each instruction instead of having
it all packed into the helper functions. Some of the "boilerplate" here
is the addition of various ISLE utilities as well.

<!--

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 18 2021 at 17:32):

alexcrichton updated PR #3545 from arm64-iadd-isub-isle to main.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Is this using flags to pass the carried bit from the add to the adc? If so, then we should have something similar to WithFlags in the x86 ISLE to use the type system to ensure that flags-producing instructions and flags-consuming instructions are always paired together, and can't accidentally be divided by another instruction that clobbers the flags.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Not adding the commutative cases yet? I don't think we need to port only exactly the existing lowerings, and then add new lowerings at some indeterminate time in the future. I think it is fine to add new optimizations/rules as we port things to ISLE.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Capitalization and punctuation? Sorry!

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Ditto here.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Although I see you did handle commutativity here. Might as well do it for the rest of the iadd lowerings too.

Wasmtime GitHub notifications bot (Nov 19 2021 at 15:08):

alexcrichton updated PR #3545 from arm64-iadd-isub-isle to main.

Wasmtime GitHub notifications bot (Nov 19 2021 at 18:08):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 18:08):

fitzgen merged PR #3545.

Wasmtime GitHub notifications bot (Nov 24 2021 at 17:13):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 24 2021 at 17:13):

cfallin created PR review comment:

Sorry for not catching this earlier -- I just noticed this while reading through the ISLE while having the language semantics in mind: this combination of rules depends on the more-specific-before-less behavior, and will give incorrect codegen if the latter rule is used when the former could apply, no?

The intention in the language design at least was that the order of rule selection is a heuristic that should go more-specific-first, for better perf/isel results, but doesn't have to. Specifically (i) it's somewhat brittle to depend on a subtle heuristic when patterns only have a partial ordering (here one of them is clearly more specific than the other, but it's possible to write two patterns with "unordered specific-ness" wrt each other), and (ii) we have some testing ideas related to randomly permuting the rewrite path we take to test if the generated code is still valid.

Another way of seeing the problem is that when we get to verifying lowerings formally, the rule here for the 64-bit case basically says "iadd_op for any type can always give Add64". Each rule standing on its own should have verifiable semantics; we shouldn't have to implicitly depend on this rule not firing in some cases because of other rules.

The priority mechanism on the other hand is solid load-bearing core-language-semantics stuff, so I think one solution here is to use explicit priorities. I'd actually prefer a slightly different solution though, which is to have another extractor (larger_than_32 ty); that makes the intent super-clear. Thoughts?

Wasmtime GitHub notifications bot (Nov 24 2021 at 17:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 24 2021 at 17:21):

cfallin created PR review comment:

(I also recognize this is a bit of a footgun that didn't come with sharp warnings, so apologies for that! In a future where we have formal verification of lowerings, or where we have rule-order fuzzing, this would've been caught fairly quickly at least...)

Last updated: Oct 23 2024 at 20:03 UTC