Stream: git-wasmtime

Topic: wasmtime / PR #7693 Cranelift: use more `iconst_[su]` in ...

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:46):

scottmcm opened PR #7693 from scottmcm:more-i128 to bytecodealliance:main:

This uses iconst_u and iconst_s a bunch more in ISLE opts.

We can now enable some of the patterns that had to be excluded before to avoid generating iconst.i128 that doesn't exist, since these rules do the right thing. For example, x - x with this PR will now give uextend.i128 (iconst.i64 0) when x is i128, rather than staying unchanged as it did before.

There's a bunch of things that still don't have it, like const-prop, since the constant values are still only 64-bit, not 128-bit, so those patterns are still restricted to the smaller types.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:46):

scottmcm requested elliottt for a review on PR #7693.

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:46):

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

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:48):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:48):

scottmcm created PR review comment:

I like how elegant this is now, but it's still restricted to 64-bit and smaller because iconst_s $I128 k will emit a sign-extended constant that's probably the same thing it started with.

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:57):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2023 at 05:57):

scottmcm created PR review comment:

I was unable to find any way to trigger this where the fits_in_64 mattered. When it's only ty_int, since it's coming from icmp it seems to always be $I8 -- anything else as the result would need a vector type, but trying with a vector type (see eq_self_v128) it's not triggering even without the 64-bit limitation.

Wasmtime GitHub notifications bot (Dec 15 2023 at 06:07):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2023 at 06:07):

scottmcm created PR review comment:

I could pull these (and the reduce ones) into a separate PR if you want, since they're not really following the bigger theme of the PR. They just came from looking at cprop -- which in general can't do 128-bit constants usefully -- and trying to find what would be possible on the extended constants that iconst_u and iconst_s generate. See the bor_extended_constants_i128 test below as an example of where this happens.

Wasmtime GitHub notifications bot (Dec 15 2023 at 08:44):

github-actions[bot] commented on PR #7693:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "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 18 2023 at 18:18):

fitzgen submitted PR review:

Looks great, thanks! Just a couple things below before merging.

Wasmtime GitHub notifications bot (Dec 18 2023 at 18:18):

fitzgen submitted PR review:

Looks great, thanks! Just a couple things below before merging.

Wasmtime GitHub notifications bot (Dec 18 2023 at 18:18):

fitzgen created PR review comment:

I think we will want to pull these out into a new, dedicated PR. It isn't obvious that these rewrites are (always? usually?) improvements on their own since we are going from two ops to three and most CPUs, AFAIK, aren't faster at doing eg 8-bit adds than 32-bit adds. I could see this possibly unlocking further rewrites, but are intentionally very careful about the kinds of rewrites that we introduce just to unlock other optimizations because we aren't working with a full e-graph here, but an acyclic one that is intended to run with less time budget. All that is to say, this might still be beneficial, but I'd like to get more eyes on it and discussion about it first, and we can land the rest of this stuff in the meantime.

Wasmtime GitHub notifications bot (Dec 18 2023 at 18:18):

fitzgen created PR review comment:

Mind adding a comment to this effect?

Wasmtime GitHub notifications bot (Dec 18 2023 at 19:20):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Dec 18 2023 at 19:20):

scottmcm created PR review comment:

Makes sense. I can also experiment with doing it only for things where one of the inner ones is itself an extend, since I think that's the only cases where I used it in the tests anyway, and would keep the excess down since the extra reduce would collapse with the extend.

Wasmtime GitHub notifications bot (Dec 18 2023 at 19:28):

scottmcm updated PR #7693.

Wasmtime GitHub notifications bot (Dec 18 2023 at 19:44):

scottmcm submitted PR review.

Wasmtime GitHub notifications bot (Dec 18 2023 at 19:44):

scottmcm created PR review comment:

Done.

Wasmtime GitHub notifications bot (Dec 18 2023 at 20:13):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Dec 18 2023 at 20:47):

fitzgen merged PR #7693.

Last updated: Dec 23 2024 at 12:05 UTC