Bug 21610: Canonicalize min/max fcmp selects to use ordered comparisons

[arsenm]

Attach correct patch

[hfinkel]

What's the motivation?

[arsenm]

In D6337#6, @hfinkel wrote:

What's the motivation?

To reduce the number of cases to inspect when looking at something that looks like a min / max. I was working on matching some different versions of min and max instructions with varying NaN behavior, and kept writing patterns permuting the operands for the different cases. Plus in the bug I noticed the temporary variable changed the IR, so this seems like a basic missing canonicalization. Also ordered compares are more 'normal' and seem to be more popular and understood in the backends, so it seems preferable to us them when possible.

[hfinkel]

Thanks, LGTM.

test/Transforms/InstCombine/unordered-fcmp-select.ll

127

Please remove unnecessary attributes.

[arsenm]

r222705

lib/Transforms/InstCombine/InstCombineSelect.cpp

if (FCI->getOperand(0) == TrueVal && FCI->getOperand(1) == FalseVal) {
         // It becomes safe if either operand is a nonzero constant.
         ConstantFP *CFPt, *CFPf;
         if (((CFPt = dyn_cast<ConstantFP>(TrueVal)) &&
               !CFPt->getValueAPF().isZero()) ||
             ((CFPf = dyn_cast<ConstantFP>(FalseVal)) &&
              !CFPf->getValueAPF().isZero()))
         return ReplaceInstUsesWith(SI, TrueVal);
       }
-      // NOTE: if we wanted to, this is where to detect MIN/MAX
 
+      // Canonicalize to use ordered comparisons by swapping the select
+      // operands.
+      //
+      // e.g.
+      // (X ugt Y) ? X : Y -> (X ole Y) ? Y : X
+      if (FCI->hasOneUse() && FCmpInst::isUnordered(FCI->getPredicate())) {
+        FCmpInst::Predicate InvPred = FCI->getInversePredicate();
+        Value *NewCond = Builder->CreateFCmp(InvPred, TrueVal, FalseVal,
+                                             FCI->getName() + ".inv");
+
+        return SelectInst::Create(NewCond, FalseVal, TrueVal,
+                                  SI.getName() + ".p");
+      }
+
+      // NOTE: if we wanted to, this is where to detect MIN/MAX
     } else if (FCI->getOperand(0) == FalseVal && FCI->getOperand(1) == TrueVal){
       // Transform (X == Y) ? Y : X  -> X
       if (FCI->getPredicate() == FCmpInst::FCMP_OEQ) {
         // This is not safe in general for floating point:
         // consider X== -0, Y== +0.
         // It becomes safe if either operand is a nonzero constant.
         ConstantFP *CFPt, *CFPf;
         if (((CFPt = dyn_cast<ConstantFP>(TrueVal)) &&
if (FCI->getOperand(0) == TrueVal && FCI->getOperand(1) == FalseVal) {
         // It becomes safe if either operand is a nonzero constant.
         ConstantFP *CFPt, *CFPf;
         if (((CFPt = dyn_cast<ConstantFP>(TrueVal)) &&
               !CFPt->getValueAPF().isZero()) ||
             ((CFPf = dyn_cast<ConstantFP>(FalseVal)) &&
              !CFPf->getValueAPF().isZero()))
           return ReplaceInstUsesWith(SI, TrueVal);
       }
+
+      // Canonicalize to use ordered comparisons by swapping the select
+      // operands.
+      //
+      // e.g.
+      // (X ugt Y) ? X : Y -> (X ole Y) ? X : Y
+      if (FCI->hasOneUse() && FCmpInst::isUnordered(FCI->getPredicate())) {
+        FCmpInst::Predicate InvPred = FCI->getInversePredicate();
+        Value *NewCond = Builder->CreateFCmp(InvPred, FalseVal, TrueVal,
+                                             FCI->getName() + ".inv");
+
+        return SelectInst::Create(NewCond, FalseVal, TrueVal,
+                                  SI.getName() + ".p");
+      }
+
       // NOTE: if we wanted to, this is where to detect MIN/MAX
     }
     // NOTE: if we wanted to, this is where to detect ABS
   }
 
   // See if we are selecting two values based on a comparison of the two values.
   if (ICmpInst *ICI = dyn_cast<ICmpInst>(CondVal))
     if (Instruction *Result = visitSelectInstWithICmp(SI, ICI))

test/Transforms/InstCombine/unordered-fcmp-select.ll

+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+; CHECK-LABEL: @select_max_ugt(
+; CHECK: %cmp.inv = fcmp ole float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %b, float %a
+; CHECK-NEXT: ret float %sel
+define float @select_max_ugt(float %a, float %b) #0 {
+  %cmp = fcmp ugt float %a, %b
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_max_uge(
+; CHECK: %cmp.inv = fcmp olt float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %b, float %a
+; CHECK-NEXT: ret float %sel
+define float @select_max_uge(float %a, float %b) #0 {
+  %cmp = fcmp uge float %a, %b
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_min_ugt(
+; CHECK: %cmp.inv = fcmp ole float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %a, float %b
+; CHECK-NEXT: ret float %sel
+define float @select_min_ugt(float %a, float %b) #0 {
+  %cmp = fcmp ugt float %a, %b
+  %sel = select i1 %cmp, float %b, float %a
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_min_uge(
+; CHECK: %cmp.inv = fcmp olt float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %a, float %b
+; CHECK-NEXT: ret float %sel
+define float @select_min_uge(float %a, float %b) #0 {
+  %cmp = fcmp uge float %a, %b
+  %sel = select i1 %cmp, float %b, float %a
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_max_ult(
+; CHECK: %cmp.inv = fcmp oge float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %a, float %b
+; CHECK-NEXT: ret float %sel
+define float @select_max_ult(float %a, float %b) #0 {
+  %cmp = fcmp ult float %a, %b
+  %sel = select i1 %cmp, float %b, float %a
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_max_ule(
+; CHECK: %cmp.inv = fcmp ogt float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %a, float %b
+; CHECK: ret float %sel
+define float @select_max_ule(float %a, float %b) #0 {
+  %cmp = fcmp ule float %a, %b
+  %sel = select i1 %cmp, float %b, float %a
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_min_ult(
+; CHECK: %cmp.inv = fcmp oge float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %b, float %a
+; CHECK-NEXT: ret float %sel
+define float @select_min_ult(float %a, float %b) #0 {
+  %cmp = fcmp ult float %a, %b
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_min_ule(
+; CHECK: %cmp.inv = fcmp ogt float %a, %b
+; CHECK-NEXT: %sel = select i1 %cmp.inv, float %b, float %a
+; CHECK-NEXT: ret float %sel
+define float @select_min_ule(float %a, float %b) #0 {
+  %cmp = fcmp ule float %a, %b
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_fcmp_une(
+; CHECK:  %cmp.inv = fcmp oeq float %a, %b
+; CHECK-NEXT:  %sel = select i1 %cmp.inv, float %b, float %a
+; CHECK-NEXT: ret float %sel
+define float @select_fcmp_une(float %a, float %b) #0 {
+  %cmp = fcmp une float %a, %b
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_fcmp_ueq
+; CHECK:  %cmp.inv = fcmp one float %a, %b
+; CHECK-NEXT:  %sel = select i1 %cmp.inv, float %b, float %a
+; CHECK-NEXT: ret float %sel
+define float @select_fcmp_ueq(float %a, float %b) #0 {
+  %cmp = fcmp ueq float %a, %b
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+declare void @foo(i1)
+
+; CHECK-LABEL: @select_max_ugt_2_use_cmp(
+; CHECK: fcmp ugt
+; CHECK-NOT: fcmp
+; CHECK: ret
+define float @select_max_ugt_2_use_cmp(float %a, float %b) #0 {
+  %cmp = fcmp ugt float %a, %b
+  call void @foo(i1 %cmp)
+  %sel = select i1 %cmp, float %a, float %b
+  ret float %sel
+}
+
+; CHECK-LABEL: @select_min_uge_2_use_cmp(
+; CHECK: fcmp uge
+; CHECK-NOT: fcmp
+; CHECK: ret
+define float @select_min_uge_2_use_cmp(float %a, float %b) #0 {
+  %cmp = fcmp uge float %a, %b
+  call void @foo(i1 %cmp)
+  %sel = select i1 %cmp, float %b, float %a
+  ret float %sel
+}
+
+attributes #0 = { nounwind readnone "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }

test/Transforms/LoopVectorize/minmax_reduction.ll

for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %max.red.0
 }
 
 ; CHECK-LABEL: @unordered_max_red_float(
-; CHECK: fcmp ugt <2 x float>
+; CHECK: fcmp ole <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp ogt <2 x float>
 ; CHECK: select i1
 
 define float @unordered_max_red_float(float %max) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %max.red.0
 }
 
 ; CHECK-LABEL: @unordered_max_red_float_ge(
-; CHECK: fcmp uge <2 x float>
+; CHECK: fcmp olt <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp ogt <2 x float>
 ; CHECK: select i1
 
 define float @unordered_max_red_float_ge(float %max) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %max.red.0
 }
 
 ; CHECK-LABEL: @inverted_unordered_max_red_float(
-; CHECK: fcmp ult <2 x float>
+; CHECK: fcmp oge <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp ogt <2 x float>
 ; CHECK: select i1
 
 define float @inverted_unordered_max_red_float(float %max) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %max.red.0
 }
 
 ; CHECK-LABEL: @inverted_unordered_max_red_float_le(
-; CHECK: fcmp ule <2 x float>
+; CHECK: fcmp ogt <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp ogt <2 x float>
 ; CHECK: select i1
 
 define float @inverted_unordered_max_red_float_le(float %max) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %min.red.0
 }
 
 ; CHECK-LABEL: @unordered_min_red_float(
-; CHECK: fcmp ult <2 x float>
+; CHECK: fcmp oge <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp olt <2 x float>
 ; CHECK: select i1
 
 define float @unordered_min_red_float(float %min) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %min.red.0
 }
 
 ; CHECK-LABEL: @unordered_min_red_float_le(
-; CHECK: fcmp ule <2 x float>
+; CHECK: fcmp ogt <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp olt <2 x float>
 ; CHECK: select i1
 
 define float @unordered_min_red_float_le(float %min) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %min.red.0
 }
 
 ; CHECK-LABEL: @inverted_unordered_min_red_float(
-; CHECK: fcmp ugt <2 x float>
+; CHECK: fcmp ole <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp olt <2 x float>
 ; CHECK: select i1
 
 define float @inverted_unordered_min_red_float(float %min) #0 {
 entry:
   br label %for.body
for.body:
   %exitcond = icmp eq i64 %indvars.iv.next, 1024
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:
   ret float %min.red.0
 }
 
 ; CHECK-LABEL: @inverted_unordered_min_red_float_ge(
-; CHECK: fcmp uge <2 x float>
+; CHECK: fcmp olt <2 x float>
 ; CHECK: select <2 x i1>
 ; CHECK: middle.block
 ; CHECK: fcmp olt <2 x float>
 ; CHECK: select i1
 
 define float @inverted_unordered_min_red_float_ge(float %min) #0 {
 entry:
   br label %for.body