[PATCH] D62158: [InstCombine] canonicalize minnum/maxnum with 'nnan' to fcmp+select

[Sanjay Patel via Phabricator via llvm-commits]

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