Stream: git-wasmtime

Topic: wasmtime / issue #6859 egraphs: Delete `select+fcmp` to `...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:37):

jameysharp commented on issue #6859:

Comparing this existing test in cranelift/filetests/filetests/runtests/fmin-pseudo.clif:

; run: %fmin_p_f32(0x0.0, +NaN) == 0x0.0

against the new test in this PR:

; run: %a(0.0, NaN) == NaN

I wouldn't think that NaN canonicalization could lead to us mixing up these results.

Oh hey, I think the rule in question was only barely wrong. I think with the arguments swapped it might work:

(rule (simplify
       (select ty (fcmp _ (FloatCC.LessThan) x y) x y))
      (fmin_pseudo ty y x))

According to the Cranelift fmin_pseudo docs, the definition is fmin_pseudo(a, b) = (b < a) ? b : a. But we were rewriting (a < b) ? a : b to fmin_pseudo(a, b).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 22:44):

github-actions[bot] commented on issue #6859:

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:

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 (Aug 21 2023 at 16:47):

jameysharp commented on issue #6859:

Okay, after thinking this through, I agree: let's remove the fmin_pseudo and fmax_pseudo instructions, and we can merge this PR as a step in that direction.

I've been thinking about what could justify keeping these instructions, given that the backends can match this pattern easily. The main case I can think of would be if they allowed us to concisely express other significant optimizations in the egraph rules. But that seems unlikely as these are rather specific. At least, I can't think of any possible examples.

Last updated: Nov 22 2024 at 16:03 UTC