[PATCH] D62158: [InstCombine] canonicalize minnum/maxnum with 'nnan' to fcmp+select
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 20 14:06:10 PDT 2019
spatel added a comment.
In D62158#1509103 <https://reviews.llvm.org/D62158#1509103>, @arsenm wrote:
> I'm not sure I see the ValueTracking advantage here. Can you add a test where this matters? I wouldn't expect matchSelectPattern to give any advantage here.
The follow-on transform that I'm thinking about is mentioned in PR37403 - if we have an offset or some other math op on both operands of a min/max, we should be able to optimize that away:
define double @foo(double, double, double) local_unnamed_addr #0 {
%4 = fadd fast double %2, %0
%5 = fadd fast double %2, %1
%6 = tail call fast double @llvm.maxnum.f64(double %4, double %5)
ret double %6
}
-->
define double @foo1(double, double, double) local_unnamed_addr #0 {
%3 = tail call fast double @llvm.maxnum.f64(double %0, double %1)
%4 = fadd fast double %2, %3
ret double %4
}
We also miss this with integer types:
define i32 @via_icmp(i32 %z, i32 %x, i32 %y) {
%xz = add nsw i32 %x, %z ; the adds must be 'nsw'
%yz = add nsw i32 %y, %z
%c = icmp slt i32 %xz, %yz
%r = select i1 %c, i32 %xz, i32 %yz
ret i32 %r
}
So there's potentially some overlap in how we match those patterns if we use matchSelectPattern(). Now, when I commented on that problem, we had codegen problems too, but I think we've overcome them. So I'd be fine with canonicalizing in the other direction:
fcmp+select --> minnum/maxnum
...but I think that's gated on D61917 <https://reviews.llvm.org/D61917> because we need 'nsz'.
> Also last I knew, the use of matchSelectPattern in SelectionDAGBuilder was somewhat buggy. It leaves behind a dead use, so in the initial combine, it breaks hasOneUse checks and prevents necessary early matching. There was also a second problem that I don't remember the details of
Ok, I didn't know there was a problem there. Do we have a bug report with an example?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62158/new/
https://reviews.llvm.org/D62158
More information about the llvm-commits
mailing list