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 isfmin_pseudo(a, b) = (b < a) ? b : a
. But we were rewriting(a < b) ? a : b
tofmin_pseudo(a, b)
.
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:
- 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>
jameysharp commented on issue #6859:
Okay, after thinking this through, I agree: let's remove the
fmin_pseudo
andfmax_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: Dec 23 2024 at 13:07 UTC