Stream: git-wasmtime

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

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:12):

cfallin commented on issue #3545:

Thanks for tackling this -- excited to see the fast progress!

I wanted to note specifically on

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

One of the things I had imagined when designing the compilation model (each internal constructor becomes a Rust function) is that we could actually replicate what we did in the handwritten code -- namely, we could have a helper that gives a ResultRSEImm12-like thing. I think doing so would help both from an expressivity point of view (less repetition, easier to understand) and code-size point of view.

An interesting question is whether this is an extractor (part of the pattern) or a constructor; IMHO it makes sense to do it as a constructor because it's infallible -- we know we can always do something, we can't fall back to other rules -- so something like (put_in_reg_shift_extend_imm12 value) would make sense to me, then that constructor can have (rule (put_in_reg_shift_extend_imm12 (imm12 val)) (ResultRSEImm12.Imm12 val)) etc. Thoughts?

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:26):

github-actions[bot] commented on issue #3545:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:33):

fitzgen commented on issue #3545:

An interesting question is whether this is an extractor (part of the pattern) or a constructor; IMHO it makes sense to do it as a constructor because it's infallible -- we know we can always do something, we can't fall back to other rules -- so something like (put_in_reg_shift_extend_imm12 value) would make sense to me, then _that_ constructor can have (rule (put_in_reg_shift_extend_imm12 (imm12 val)) (ResultRSEImm12.Imm12 val)) etc. Thoughts?

Because addition is commutative, we want to use a fallible extractor here that we can try in both the left and right operands, and then have a fallback base case for putting everything into registers.

If this were subtraction, and we would want to just call this once for the right operand and accept whatever answer it gave, then I would agree that it should be a plain constructor.

This is analogous to why there is both put_in_reg_mem and the sinkable_load extractor and sink_load constructor in the x86 ISLE code.

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:34):

fitzgen edited a comment on issue #3545:

An interesting question is whether this is an extractor (part of the pattern) or a constructor; IMHO it makes sense to do it as a constructor because it's infallible -- we know we can always do something, we can't fall back to other rules -- so something like (put_in_reg_shift_extend_imm12 value) would make sense to me, then _that_ constructor can have (rule (put_in_reg_shift_extend_imm12 (imm12 val)) (ResultRSEImm12.Imm12 val)) etc. Thoughts?

Because addition is commutative, we want to use a fallible extractor here -- one that doesn't ever put the value into a register, only the immediate or shift or extend operand forms -- that we can try in both the left and right operands, and then have a fallback base case for putting everything into registers if those preferred operand modes fail.

If this were subtraction, and we would want to just call this once for the right operand and accept whatever answer it gave, then I would agree that it should be a plain constructor.

This is analogous to why there is both put_in_reg_mem and the sinkable_load extractor and sink_load constructor in the x86 ISLE code.

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:35):

fitzgen edited a comment on issue #3545:

An interesting question is whether this is an extractor (part of the pattern) or a constructor; IMHO it makes sense to do it as a constructor because it's infallible -- we know we can always do something, we can't fall back to other rules -- so something like (put_in_reg_shift_extend_imm12 value) would make sense to me, then _that_ constructor can have (rule (put_in_reg_shift_extend_imm12 (imm12 val)) (ResultRSEImm12.Imm12 val)) etc. Thoughts?

Because addition is commutative, we want to use a fallible extractor here -- one that doesn't ever put the value into a register, only the immediate or shift or extend operand forms -- that we can try in both the left and right operands, and then have a fallback base case for putting everything into registers if those preferred operand modes fail.

If this were subtraction, and we would want to just call this once for the right operand and accept whatever answer it gave, then I would agree that it should be a plain constructor.

This is analogous to why there is both put_in_reg_mem and the sinkable_load extractor and sink_load constructor pair in the x86 ISLE code.

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:43):

alexcrichton commented on issue #3545:

This is definitely one of the things I went back and forth on a lot of how to design this support. I was originally thinking I would do something like:

(rule (lower (has_type (fits_in_64 ty) (iadd x (extract_rse_imm12 y))))
      (value_reg (alu_rse (iadd_op ty) (put_in_reg x) y)))

where extract_rse_imm12 basically corresponds to put_input_in_rse_imm. This gets kinda unfortunate I think relatively quickly for a few reasons:

• One is that for add/subtraction we actually want extract_rse_maybe_negated_imm12 which has two outputs which is sorta wonky to describe in ISLE.
• Currently I think extract_rse_imm12 can only be implemented in Rust, not in ISLE itself, which means that ISLE has no visibility into trees of expressions for a better trie to build. This also means that a lot of "pattern matching code" would still be present in Rust which I figured the goal was to move most of it to ISLE.

My end-conclusion was that for all the put_input_in_rs*-style functions we'd have custom extractors for each variant of the rs* (one for extending, one for shifting, one for immediates, etc). This gives ISLE more visibility into the entire trie. For cases like shifts and immediates ISLE has the entire trie, but for extends ISLE doesn't get any part of the trie because we want to match a suite of extending operations and ISLE extractors can only have one "arm" right now in a sense. But I was hopeful that maybe one day this could be worked out where ISLE has the whole trie and is able to optimize with that where Rust is doing the bare minimum of converting between immediates and such.

My conclusion was also the same as Nick's though where we want to have these as extractors rather than constructors. Today the AArch64 backend doesn't actually handle the case of an iadd with a left-hand-side immediate, that generates movement instructions and such. I think we might want to feasibly handle the commutative sides as well here in lowering.

Wasmtime GitHub notifications bot (Nov 18 2021 at 18:06):

cfallin commented on issue #3545:

Ah, good points. I agree that given the need to try both operand orders and pick the better one, and given subtle variations in some instructions, the current approach probably makes the most sense.

One thought that does strike me is: if there are instructions with exactly the same cases, modulo opcode itself (e.g., all of the logical insts -- and, or, xor as a group, and and-not / or-not / xor-not as a group), we could have an extractor that matches any of the group, and returns the opcode; then use this opcode in the (alu_rr_immlogical ...) or whatnot. Something like:

(rule (lower (aarch64_logical_inst op ...))
          (alu_rr_immlogical op (put_in_reg ...) (put_in_reg ...)))

with the N cases for immediates, shifts, etc., laid out here but only once for the and/or/not/... group.

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

alexcrichton commented on issue #3545:

Ah yeah I want to stay on the lookout for whether iadd/isub will have lots of duplication with other instructions, and if that's the case I think it's worth figuring out a different route here like the aarch64_logical_inst you mentioned @cfallin.

Otherwise should be updated with your review @fitzgen!

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

fitzgen commented on issue #3545:

Thanks!

Last updated: Nov 22 2024 at 16:03 UTC