Wasmtime GitHub notifications bot (Nov 30 2023 at 07:59): Wasmtime GitHub notifications bot (Nov 30 2023 at 07:59):scottmcm opened PR #7615 from scottmcm:spaceship to bytecodealliance:main:
I was writing a pattern for Rust's
Ord::cmp(context zulip thread)And noticed that the
if *self == *other { Equal } else { Greater }part isif c { false } else { true }.So here's a smaller PR with some simpler sub-patterns first.
Wasmtime GitHub notifications bot (Nov 30 2023 at 07:59):scottmcm requested elliottt for a review on PR #7615.
Wasmtime GitHub notifications bot (Nov 30 2023 at 07:59):scottmcm requested wasmtime-compiler-reviewers for a review on PR #7615.
Wasmtime GitHub notifications bot (Nov 30 2023 at 08:43):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2023 at 08:43):scottmcm created PR review comment:
Is the duplicated rules -- one for I8, one for wider -- the best way to do this? Or is there a
uextend-if-neededthat one rule could produce?
Wasmtime GitHub notifications bot (Nov 30 2023 at 08:45):github-actions[bot] commented on PR #7615:
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 (Nov 30 2023 at 16:30):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2023 at 16:30):alexcrichton created PR review comment:
There's various little helpers around and I'm not super familiar with these ISLE files, but it's fine to add a helper for this. I'd also recommend doing that because this rule you've commented on will produce invalid CLIF if
styis$I8becauseuextendis only valid of the output type is larger than the input type.A small helper along the lines of this though would fix it:
(decl uextend_to_ty (Type Value) Value) (rule 0 (uextend_to_ty ty val) (uextend ty val)) (rule 1 (uextend_to_ty $I8 val) val)
Wasmtime GitHub notifications bot (Nov 30 2023 at 16:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2023 at 16:32):alexcrichton created PR review comment:
To further expand on this a bit as well, what you've defined here are effectively two overlapping rules, for example the rule on line 23 (this one) and the rule on line 13 (the i8 case above). For an i8-producing-select both rules match, and I believe ISLE allows that for the "multi" constructor that
simplifyis (since I believe it's generating all matches and yielding them to the egraph driver). While the above rule is "lower cost" and will get selected eventually I think it's probably still best to avoid the possibility of accidentally creating invalid CLIF
Wasmtime GitHub notifications bot (Nov 30 2023 at 18:45):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2023 at 18:45):scottmcm created PR review comment:
Ah, got it. When having the
$I8pattern meant that the tests worked for both widths, I didn't know if that meant it was treated as "more specific" or something.I'll add (or find) a helper for this. I agree having the useless-and-invalid
extendoption in the graph isn't good.
Wasmtime GitHub notifications bot (Nov 30 2023 at 19:25):scottmcm updated PR #7615.
Wasmtime GitHub notifications bot (Nov 30 2023 at 19:37):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2023 at 19:37):alexcrichton has enabled auto merge for PR #7615.
Wasmtime GitHub notifications bot (Nov 30 2023 at 19:50):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2023 at 19:50):scottmcm created PR review comment:
Thanks! I couldn't find any similar existing rule, but the one you provided worked great.
Wasmtime GitHub notifications bot (Nov 30 2023 at 20:05):alexcrichton merged PR #7615.
Last updated: Dec 23 2024 at 12:05 UTC