Stream: git-wasmtime

Topic: wasmtime / PR #7670 Cranelift: Add iconst shorthand to si...


view this post on Zulip Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20):

scottmcm requested abrown for a review on PR #7670.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20):

scottmcm requested wasmtime-compiler-reviewers for a review on PR #7670.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 09 2023 at 10:20):

scottmcm opened PR #7670 from scottmcm:iconst_su to bytecodealliance:main:

Extracting an iconst with value 1 today is easy enough, albeit needing more type-conversions than one might like

https://github.com/bytecodealliance/wasmtime/blob/4f2d634ca4291a09003eaba26f989cd544c1a289/cranelift/codegen/src/opts/arithmetic.isle#L52-L56

However there's no way to extract against a type-aware -1, meaning those patterns have an extra step

https://github.com/bytecodealliance/wasmtime/blob/4f2d634ca4291a09003eaba26f989cd544c1a289/cranelift/codegen/src/opts/arithmetic.isle#L64-L67

This PR adds iconst_s ty c that can directly extract a sign-extended value as well as construct a properly masked constant (so iconst_s ty -1 will give iconst.i8 255 or iconst.i16 0xFFFF or …). And for symmetry, it also adds iconst_u, which also works as both constructor and extractor.

Thus the pattern looking for -1 is 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_imm64 vs imm64 correct depending whether something is extracting or constructing, since iconst_u works 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 simm32 and uimm8 to 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 simm32 is implemented.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 09 2023 at 10:22):

scottmcm updated PR #7670.

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

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 15:32):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 15:32):

alexcrichton submitted PR review.

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

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 18:49):

scottmcm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 18:49):

scottmcm created PR review comment:

I would love to do that -- manually loading the InstructionData is something I'd much rather avoid -- but I couldn't find a way to do it.

First, i64_sextend_imm64 is 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 an Imm64 of 255 is -1_i64 or 255_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.dfg stuff, 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 new externs if I can help it. I just couldn't find the way to do that for iconst_s.)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 19:02):

scottmcm submitted PR review.

view this post on Zulip 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 of Value, but that needs a way to write a pattern like (inst_data (new_thing c)) even though inst_data needs 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

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 19:56):

scottmcm updated PR #7670.

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

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 19:58):

scottmcm updated PR #7670.

view this post on Zulip 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_data code (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 tupled versions of every single instruction -- so you could have (iconst_tupled (my_extractor …)) -- but I'm not going that far right now :upside_down:

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Dec 12 2023 at 20:31):

fitzgen merged PR #7670.


Last updated: Dec 23 2024 at 12:05 UTC