Stream: git-wasmtime

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


view this post on Zulip 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:

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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2021 at 17:32):

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen submitted PR review.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Capitalization and punctuation? Sorry!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2021 at 19:14):

fitzgen created PR review comment:

Ditto here.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2021 at 15:08):

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2021 at 18:08):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2021 at 18:08):

fitzgen merged PR #3545.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 24 2021 at 17:13):

cfallin submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 24 2021 at 17:21):

cfallin submitted PR review.

view this post on Zulip 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: Dec 23 2024 at 13:07 UTC