Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20): Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20):scottmcm requested abrown for a review on PR #7670.
Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20):scottmcm requested wasmtime-compiler-reviewers for a review on PR #7670.
Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20):scottmcm opened PR #7670 from scottmcm:iconst_su to bytecodealliance:main:
Extracting an
iconstwith value1today is easy enough, albeit needing more type-conversions than one might likeHowever there's no way to extract against a type-aware
-1, meaning those patterns have an extra stepThis PR adds
iconst_s ty cthat can directly extract a sign-extended value as well as construct a properly masked constant (soiconst_s ty -1will giveiconst.i8 255oriconst.i16 0xFFFFor …). And for symmetry, it also addsiconst_u, which also works as both constructor and extractor.Thus the pattern looking for
-1is simplified to just;; x*-1 == ineg(x). (rule (simplify (imul ty x (iconst_s ty -1))) (ineg ty x))And even unsigned cases are shortened and simplified by no longer needing to remember to get
u64_from_imm64vsimm64correct depending whether something is extracting or constructing, sinceiconst_uworks for both;; uge(x, 0) == true. -(rule (simplify (uge (fits_in_64 (ty_int bty)) x zero @ (iconst _ (u64_from_imm64 0)))) - (subsume (iconst bty (imm64 1)))) +(rule (simplify (uge (fits_in_64 (ty_int bty)) x zero @ (iconst_u _ 0))) + (subsume (iconst_u bty 1)))This makes no functional changes to the patterns, just makes them shorter using the new etor+ctors, and thus there are no test changes. (There's way too many changes in the ISLE for it to be a good idea to try to also change any functionality here. Similarly, to attempt to ease review, I did no formatting changes in the ISLE, even where I might have otherwise removed some line-breaking.)
This also moves
simm32anduimm8to lowering ISLE only, as they appear not actually useful for opts ISLE.In particular, it's very tempting to write
;; x*-1 == ineg(x). (rule (simplify (imul ty x (iconst _ (simm32 -1)))) (ineg ty x))but despite its name, that doesn't even work for 32-bit numbers because of how
simm32is implemented.
Wasmtime GitHub notifications bot (Dec 09 2023 at 10:22):scottmcm updated PR #7670.
Wasmtime GitHub notifications bot (Dec 09 2023 at 12:48):github-actions[bot] commented on PR #7670:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "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 (Dec 11 2023 at 15:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 11 2023 at 15:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 11 2023 at 15:32):alexcrichton created PR review comment:
I think this might actually be able to get replicated into ISLE with
(extractor iconst_s (iconst ty (i64_sextend_imm64 n))), although I forget the precise syntax. Effectively though it should be possible to build up this new extractor based on previous extractors I think?
Wasmtime GitHub notifications bot (Dec 11 2023 at 18:49):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (Dec 11 2023 at 18:49):scottmcm created PR review comment:
I would love to do that -- manually loading the
InstructionDatais something I'd much rather avoid -- but I couldn't find a way to do it.First,
i64_sextend_imm64is a constructor, not an extractor, so it needs something extra over what already exists.So then I tried to just add something like
i64_sextend_imm64_etor. The problem I hit with that is that it needs the type information -- that's the only way to know whether anImm64of 255 is-1_i64or255_i64.That means it would need to be something like
(extractor iconst_s (iconst ty (i64_sextend_imm64_etor ty n)))But if you try to define that extractor, you get a signature of
fn(&mut self, Imm64) -> Option<(Type, i64)>, which is impossible to implement. I couldn't find a way to have what I guess would have to be sortof a "parameterized extractor" so that it can take one of its parameters as an input and the other as an output -- but it feels like that might fundamentally break the "there are extractors and there are constructors" model that it seems like ISLE depends on.Thus the only way I could figure out to make it work was the version where the extractor has the signature
fn(&mut self, Value) -> Option<(Type, i64)>that you see here.So I'd love to not need the
self.ctx.func.dfgstuff, but I couldn't figure out how. Do let me know if you have any alternatives I could try.(Said otherwise, you'll notice that
iconst_u*does* use that approach of building it up from other extractors, since zero-extending is easier, as I agree I'd much rather do that than add newexterns if I can help it. I just couldn't find the way to do that foriconst_s.)
Wasmtime GitHub notifications bot (Dec 11 2023 at 19:02):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (Dec 11 2023 at 19:02):scottmcm created PR review comment:
Hmm, looking at trying to clean it up slightly, is there such a thing as multiple-output extractors? The extern could be much simpler if it got
(Type, InstructionData)instead ofValue, but that needs a way to write a pattern like(inst_data (new_thing c))even thoughinst_dataneeds two parameters, and I don't know how to write that.Basic attempts say that it's not allowed even syntactically:
Error building ISLE files: parse error: Unexpected token LParen src\prelude_opt.isle:85:26: (decl my_new_thing (i64) (Type InstructionData)) ^(Irrelevant aside: things like this are why I find concatenative languages really elegant conceptually, since for them multiple return values work as smoothly as multiple arguments
Wasmtime GitHub notifications bot (Dec 11 2023 at 19:56):scottmcm updated PR #7670.
Wasmtime GitHub notifications bot (Dec 11 2023 at 19:57):scottmcm commented on PR #7670:
There we go; hopefully that's better. By manually uncurrying things I could still use the same
inst_datacode (albeit under a different name to make the ISLE types work out) so it does more work in ISLE and less work in the extractor itself.
Wasmtime GitHub notifications bot (Dec 11 2023 at 19:58):scottmcm updated PR #7670.
Wasmtime GitHub notifications bot (Dec 11 2023 at 20:07):scottmcm edited a comment on PR #7670:
There we go; hopefully that's better. By manually uncurrying things I could still use the same
inst_datacode (albeit under a different name to make the ISLE types work out) so it does more work in ISLE and less work in the extractor itself.I suppose there's a version of this that instead makes
tupledversions of every single instruction -- so you could have(iconst_tupled (my_extractor …))-- but I'm not going that far right now :upside_down:
Wasmtime GitHub notifications bot (Dec 12 2023 at 15:50):alexcrichton submitted PR review:
Thanks for pushing on that! You're reaching my own personal limit of knowledge of extractors and ISLE as well as I've run into the same things myself.
@fitzgen would you be ok taking a look at this too? You're more familiar with ISLE than I and you might spot something I'm missing.
Wasmtime GitHub notifications bot (Dec 12 2023 at 19:57):fitzgen submitted PR review:
Fantastic! We've wanted to do this kind of thing for a long time (couple constant values to their type and signedness) but no one ever had time or stepped up. Thanks for doing this!
Wasmtime GitHub notifications bot (Dec 12 2023 at 20:31):fitzgen merged PR #7670.
Last updated: Nov 22 2024 at 16:03 UTC