Wasmtime GitHub notifications bot (Dec 16 2021 at 16:23): Wasmtime GitHub notifications bot (Dec 16 2021 at 16:23):alexcrichton opened PR #3608 from isle-7-ushr to main:
This commit migrates the
ishl,ushr, andsshrinstructions to
ISLE. These involve special cases for almost all types of integers
(including vectors) and helper functions for the i128 lowerings since
the i128 lowerings look to be used for other instructions as well. This
doesn't delete the i128 lowerings in the Rust code just yet because
they're still used by Rust lowerings, but they should be deletable in
due time once those lowerings are translated to ISLE.<!--
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 (Dec 16 2021 at 18:36):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:36):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:36):cfallin created PR review comment:
I think it'd probably be good to build out the
ProducesFlags/ConsumesFlagstype wrappers initially, just to avoid the cost later of undoing the "temporary manually-managed-for-now flags usage" approach. It's hopefully not more than 10-20 lines of ISLE? If it turns out to be too difficult for some reason then of course we can reconsider...
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:36):cfallin created PR review comment:
Same comment here as above for the temp names (thanks!).
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:36):cfallin created PR review comment:
This sequence (copied from the original non-SSA code) reuses temps, so it's a bit confusing in context here... could you rewrite it in terms of the variable binding names in the
letbelow?
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:36):cfallin created PR review comment:
This is definitely a pre-existing thing, but reading through this code it's becoming pretty apparent to me that having helpers to be able to write e.g.
(lsl64 src lo_amt)rather than(alu_rrr (ALUOp.Lsl64) ...)would make things a lot more readable. I knowALUOpcan be used in multiple instruction formats so we have duplication (lsl64 with immediate vs not, add64 with immediate, extend, shift, ...) but it seems reasonable to me to pay that cost in order to have super-clear lowering patterns. Thoughts? (This is out-of-scope for this PR of course, would be a followup refactor if we do it)
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:43):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 18:43):alexcrichton created PR review comment:
I originally started to do that but the idioms of the AArch64 backend are what stopped me. On x86 each instruction seems to largely be used with one variant of the
Instenum, but on AArch64 oneALUOpcan be used with multiple variants (as you've pointed out).Basically I wasn't sure how to resolve that. I figured that going down the route of
alu_rrrinst builders was not horrible but I agree it's not exactly the most readable. I'd be happy to change though if you've got an idea of how to name the constructors or resolve this
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:15):alexcrichton updated PR #3608 from isle-7-ushr to main.
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:15):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:15):alexcrichton created PR review comment:
Sure thing! I believe the most recent commit should take care of this
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:44):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:44):cfallin created PR review comment:
Maybe something like: just the mnemonic for the all-regs case (so
(add64 rn rm),(lsl64 rn rm), etc.), and then e.g.add64_imm12,add64_shift,add64_extend? This also fixes the issue where illegal combinations are allowed currently: e.g. right now we can put the pieces together to make anadd64withImmLogicbut that's disallowed (should hit a panic during emission) -- better to catch statically.Not super-urgent, just better to do sooner than later if we're going to do it!
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:53):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 19:53):cfallin created PR review comment:
Interesting approach here! I kind of like the functional design (flags consumer just takes a flags producer) vs. what we have in x64 (separate toplevel combinator takes a consumer and producer).
That said, this abstraction won't allow us to reuse flags later, unless I'm missing some approach -- e.g. we couldn't have two separate
csels, or acseland anadc, or something like that, both use one compare. That was part of the reasoning for the top-level combinator approach. I'd also sort of prefer to be consistent if we can, just so folks jumping back and forth between the machine backends don't have to shift mindset more than necessary (admittedly this purely aspirational and we have plenty of differences in the handwritten code).Was there a reason you had in mind to prefer this approach over the x64-style one, or just preference? If not, and if it's not too much trouble (and sorry for pushing a bit more on this!), I think it might be good to take the x64-style approach... thanks!
Wasmtime GitHub notifications bot (Dec 16 2021 at 20:21):alexcrichton updated PR #3608 from isle-7-ushr to main.
Wasmtime GitHub notifications bot (Dec 16 2021 at 20:21):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 20:21):alexcrichton created PR review comment:
Oh when I originally looked at
ProducesFlagsit didn't seem right because it has aRegpayload and I thought thattstdidn't produce any registers so it was intened for something else. After reading how this is used with x64 though I see that the field can be(invalid_reg)which means I was able to switch over to the existing functions inprelude.islefor this.
Wasmtime GitHub notifications bot (Dec 16 2021 at 20:26):alexcrichton updated PR #3608 from isle-7-ushr to main.
Wasmtime GitHub notifications bot (Dec 16 2021 at 20:37):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Dec 16 2021 at 21:43):alexcrichton updated PR #3608 from isle-7-ushr to main.
Wasmtime GitHub notifications bot (Dec 16 2021 at 22:48):alexcrichton updated PR #3608 from isle-7-ushr to main.
Wasmtime GitHub notifications bot (Dec 16 2021 at 23:37):alexcrichton merged PR #3608.
Last updated: Dec 23 2024 at 12:05 UTC