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 ↗

(On Diff #16420)

Please remove unnecessary attributes.

[arsenm]

r222705

lib/Transforms/InstCombine/InstCombineCompares.cpp

if (Instruction *LHSI = dyn_cast<Instruction>(Op0))
           if (GlobalVariable *GV = dyn_cast<GlobalVariable>(GEP->getOperand(0)))
             if (GV->isConstant() && GV->hasDefinitiveInitializer() &&
                 !cast<LoadInst>(LHSI)->isVolatile())
               if (Instruction *Res = FoldCmpLoadFromIndexedGlobal(GEP, GV, I))
                 return Res;
         }
         break;
       case Instruction::Call: {
+        if (!RHSC->isNullValue())
+          break;
+
         CallInst *CI = cast<CallInst>(LHSI);
-        LibFunc::Func Func;
+        const Function *F = CI->getCalledFunction();
+        if (!F)
+          break;
+
         // Various optimization for fabs compared with zero.
-        if (RHSC->isNullValue() && CI->getCalledFunction() &&
-            TLI->getLibFunc(CI->getCalledFunction()->getName(), Func) &&
-            TLI->has(Func)) {
-          if (Func == LibFunc::fabs || Func == LibFunc::fabsf ||
-              Func == LibFunc::fabsl) {
-            switch (I.getPredicate()) {
-            default: break;
+        LibFunc::Func Func;
+        if (F->getIntrinsicID() == Intrinsic::fabs ||
+            (TLI->getLibFunc(F->getName(), Func) && TLI->has(Func) &&
+             (Func == LibFunc::fabs || Func == LibFunc::fabsf ||
+              Func == LibFunc::fabsl))) {
+          default:
+            break;
             // fabs(x) < 0 --> false
-            case FCmpInst::FCMP_OLT:
-              return ReplaceInstUsesWith(I, Builder->getFalse());
             // fabs(x) > 0 --> x != 0
-            case FCmpInst::FCMP_OGT:
-              return new FCmpInst(FCmpInst::FCMP_ONE, CI->getArgOperand(0),
-                                  RHSC);
+            return new FCmpInst(FCmpInst::FCMP_ONE, CI->getArgOperand(0), RHSC);
             // fabs(x) <= 0 --> x == 0
-            case FCmpInst::FCMP_OLE:
-              return new FCmpInst(FCmpInst::FCMP_OEQ, CI->getArgOperand(0),
-                                  RHSC);
+            return new FCmpInst(FCmpInst::FCMP_OEQ, CI->getArgOperand(0), RHSC);
             // fabs(x) >= 0 --> !isnan(x)
-            case FCmpInst::FCMP_OGE:
-              return new FCmpInst(FCmpInst::FCMP_ORD, CI->getArgOperand(0),
-                                  RHSC);
+            return new FCmpInst(FCmpInst::FCMP_ORD, CI->getArgOperand(0), RHSC);
             // fabs(x) == 0 --> x == 0
             // fabs(x) != 0 --> x != 0
-            case FCmpInst::FCMP_OEQ:
-            case FCmpInst::FCMP_UEQ:
-            case FCmpInst::FCMP_ONE:
-            case FCmpInst::FCMP_UNE:
-              return new FCmpInst(I.getPredicate(), CI->getArgOperand(0),
-                                  RHSC);
-            }
+            return new FCmpInst(I.getPredicate(), CI->getArgOperand(0), RHSC);
           }
         }
       }
       }
   }
 
   // fcmp pred (fneg x), (fneg y) -> fcmp swap(pred) x, y
   Value *X, *Y;

test/Transforms/InstCombine/fcmp.ll

 ; RUN: opt -S -instcombine < %s | FileCheck %s
 
+declare double @llvm.fabs.f64(double) nounwind readnone
+
 define i1 @test1(float %x, float %y) nounwind {
   %ext1 = fpext float %x to double
   %ext2 = fpext float %y to double
   %cmp = fcmp ogt double %ext1, %ext2
   ret i1 %cmp
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT: fcmp ogt float %x, %y
 }
define i32 @test9(double %a) nounwind {
   %cmp = fcmp olt double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test9(
 ; CHECK-NOT: fabs
 ; CHECK: ret i32 0
 }
 
+define i32 @test9_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp olt double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test9_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: ret i32 0
+}
+
 define i32 @test10(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp ole double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test10(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp oeq double %a, 0.000000e+00
 }
 
+define i32 @test10_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp ole double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test10_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp oeq double %a, 0.000000e+00
+}
+
 define i32 @test11(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp ogt double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test11(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp one double %a, 0.000000e+00
 }
 
+define i32 @test11_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp ogt double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test11_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp one double %a, 0.000000e+00
+}
+
 define i32 @test12(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp oge double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test12(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp ord double %a, 0.000000e+00
 }
 
+define i32 @test12_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp oge double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test12_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp ord double %a, 0.000000e+00
+}
+
 define i32 @test13(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp une double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test13(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp une double %a, 0.000000e+00
 }
 
+define i32 @test13_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp une double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test13_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp une double %a, 0.000000e+00
+}
+
 define i32 @test14(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp oeq double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test14(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp oeq double %a, 0.000000e+00
 }
 
+define i32 @test14_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp oeq double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test14_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp oeq double %a, 0.000000e+00
+}
+
 define i32 @test15(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp one double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test15(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp one double %a, 0.000000e+00
 }
 
+define i32 @test15_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp one double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test15_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp one double %a, 0.000000e+00
+}
+
 define i32 @test16(double %a) nounwind {
   %call = tail call double @fabs(double %a) nounwind
   %cmp = fcmp ueq double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 ; CHECK-LABEL: @test16(
 ; CHECK-NOT: fabs
 ; CHECK: fcmp ueq double %a, 0.000000e+00
 }
 
+define i32 @test16_intrinsic(double %a) nounwind {
+  %call = tail call double @llvm.fabs.f64(double %a) nounwind
+  %cmp = fcmp ueq double %call, 0.000000e+00
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+; CHECK-LABEL: @test16_intrinsic(
+; CHECK-NOT: fabs
+; CHECK: fcmp ueq double %a, 0.000000e+00
+}
+
 ; Don't crash.
 define i32 @test17(double %a, double (double)* %p) nounwind {
   %call = tail call double %p(double %a) nounwind
   %cmp = fcmp ueq double %call, 0.000000e+00
   %conv = zext i1 %cmp to i32
   ret i32 %conv
 }