Kmeakin edited PR #6072 from ineg-iabs
to main
:
;; iabs(ineg(x)) == iabs(x). ;; iabs(iabs(x)) == x.
elliottt submitted PR review.
elliottt created PR review comment:
Is this true? If
x
is-1
won't this rewrite to-1
whenabs(abs -1)
will be1
?
elliottt submitted PR review.
elliottt created PR review comment:
Oh, reading the rule I think it's just the comment and PR description that are wrong. It's actually:
iabs (iabs(x)) == iabs(x)
elliottt edited PR review comment.
jameysharp submitted PR review.
jameysharp created PR review comment:
There have been some discussions about
subsume
and how to use it well. We're still not sure, because it's complicated.For example, here you aren't just claiming that
iabs(x)
is better thaniabs(ineg(x))
, you're also claiming that it's better than every other rule that matches here. Maybe that's true, but I don't know how to convince myself either way.Using
subsume
never causes the optimizer to produce incorrect results, so using it here isn't wrong. It just cuts off the search for other results which might have been better.My current opinion is that we should only use
subsume
when rewriting to a constant (where it's easy to prove that there is no better alternative) and _maybe_ when rewriting to a term which came directly from the pattern on the left-hand side of the rule, like you did below with iabs/iabs. I've had trouble convincing myself of even that though.I missed reviewing your PRs like the
-x*-y
rewrite immediately above this, and that's okay. I'm not going to ask you to remove the subsumes from those; it's not that big a deal. But would you remove this one?
Kmeakin submitted PR review.
Kmeakin created PR review comment:
Good catch!
Kmeakin updated PR #6072 from ineg-iabs
to main
.
Kmeakin requested jameysharp for a review on PR #6072.
jameysharp submitted PR review.
jameysharp has enabled auto merge for PR #6072.
jameysharp merged PR #6072.
Last updated: Dec 23 2024 at 13:07 UTC