InstCombine: fix fold "fcmp x, undef" to account for NaN

[mehdi_amini]

Summary

See the two test cases.

; Can fold fcmp with undef on one side by choosing NaN for the undef

; Can fold fcmp with undef on both side
; fcmp u_pred undef, undef -> true
; fcmp o_pred undef, undef -> false
; because whatever you choose for the first undef
; you can choose NaN for the other undef

[hfinkel]

I wonder if we could access the target machine specific info to know
if NaN are disabled?
Or what is the right way of getting the -enable-no-nans-fp-math in opt?

Check the fast-math flags on the the operand (call Op->getFastMathFlags()). The LangRef says:

LLVM IR floating-point binary ops (fadd, fsub, fmul, fdiv, frem) have the following flags that can set to enable otherwise unsafe floating point operations
...
nnan - No NaNs - Allow optimizations to assume the arguments and result are not NaN. Such optimizations are required to retain defined behavior over NaNs, but the value of the result is undefined.
...
fast - Fast - Allow algebraically equivalent transformations that may dramatically change results in floating point (e.g. reassociate). This flag implies all the others.

For constant values, you can call C->isNaN();

[mehdi_amini]

After more thought, I found out we can fold depending on if fcmp is or isn't ordered.

[mehdi_amini]

Ping.

[mehdi_amini]

Ping. Can someone look at this InstCombine change?

[chandlerc]

CC-ing David who seems like he might be in a good position to review this.

[majnemer]

lib/Analysis/InstructionSimplify.cpp

3044

It would be nice to have LHS on the left and RHS on the right.

I think the comment would be clearer if it mentioned that this simplification will effect fcmp pred undef, x as well.

3045

Isn't Pred already a CmpInst::Predicate? What does this line achieve?

3047

s/fails/fail/

lib/IR/ConstantFold.cpp

1330

perhaps "use the standard constant folder."

1673–1675

Either move the ICmpInst::isIntPredicate(Predicate) && isa<UndefValue>(C1) && isa<UndefValue>(C2) check out of this block or simplify it with ICmpInst::isIntPredicate(Predicate) && C1 == C2, the line is getting a bit too long.

[mehdi_amini]

Update thanks to David's review.

[majnemer]

LGTM

include/llvm/IR/Instructions.h

assert(getOperand(0)->getType() == getOperand(1)->getType() &&
            "Both operands to FCmp instruction are not of the same type!");
     // Check that the operands are the right type
     assert(getOperand(0)->getType()->isFPOrFPVectorTy() &&
            "Invalid operand types for FCmp instruction");
   }
 
   /// @returns true if the predicate of this instruction is EQ or NE.
   /// \brief Determine if this is an equality predicate.
-  bool isEquality() const {
-    return getPredicate() == FCMP_OEQ || getPredicate() == FCMP_ONE ||
-           getPredicate() == FCMP_UEQ || getPredicate() == FCMP_UNE;
+  static bool isEquality(Predicate Pred) {
+    return Pred == FCMP_OEQ || Pred == FCMP_ONE || Pred == FCMP_UEQ ||
+           Pred == FCMP_UNE;
   }
 
+  /// @returns true if the predicate of this instruction is EQ or NE.
+  /// \brief Determine if this is an equality predicate.
+  bool isEquality() const { return isEquality(getPredicate()); }
+
   /// @returns true if the predicate of this instruction is commutative.
   /// \brief Determine if this is a commutative predicate.
   bool isCommutative() const {
     return isEquality() ||
            getPredicate() == FCMP_FALSE ||
            getPredicate() == FCMP_TRUE ||
            getPredicate() == FCMP_ORD ||
            getPredicate() == FCMP_UNO;

lib/Analysis/InstructionSimplify.cpp

static Value *SimplifyFCmpInst(unsigned Predicate, Value *LHS, Value *RHS,
   }
 
   // Fold trivial predicates.
   if (Pred == FCmpInst::FCMP_FALSE)
     return ConstantInt::get(GetCompareTy(LHS), 0);
   if (Pred == FCmpInst::FCMP_TRUE)
     return ConstantInt::get(GetCompareTy(LHS), 1);
 
-  if (isa<UndefValue>(RHS))                  // fcmp pred X, undef -> undef
-    return UndefValue::get(GetCompareTy(LHS));
+  // fcmp pred x, undef
+  if (isa<UndefValue>(RHS) || isa<UndefValue>(LHS)) {
+    CmpInst::Predicate Predicate = CmpInst::Predicate(Pred);
+    // Choosing NaN for the undef will always make unordered comparison succeed
+    // and ordered comparison fails.
+    return ConstantInt::get(GetCompareTy(LHS), CmpInst::isUnordered(Predicate));
+  }
 
   // fcmp x,x -> true/false.  Not all compares are foldable.
   if (LHS == RHS) {
     if (CmpInst::isTrueWhenEqual(Pred))
       return ConstantInt::get(GetCompareTy(LHS), 1);
     if (CmpInst::isFalseWhenEqual(Pred))
       return ConstantInt::get(GetCompareTy(LHS), 0);
   }

lib/IR/ConstantFold.cpp

static FCmpInst::Predicate evaluateFCmpRelation(Constant *V1, Constant *V2) {
   assert(V1->getType() == V2->getType() &&
          "Cannot compare values of different types!");
 
   // Handle degenerate case quickly
   if (V1 == V2) return FCmpInst::FCMP_OEQ;
 
   if (!isa<ConstantExpr>(V1)) {
     if (!isa<ConstantExpr>(V2)) {
-      // We distilled thisUse the standard constant folder for a few cases
+      // We distilled this down to a simple case, use standard constant folder.
       ConstantInt *R = nullptr;
       R = dyn_cast<ConstantInt>(
                       ConstantExpr::getFCmp(FCmpInst::FCMP_OEQ, V1, V2));
       if (R && !R->isZero()) 
         return FCmpInst::FCMP_OEQ;
       R = dyn_cast<ConstantInt>(
                       ConstantExpr::getFCmp(FCmpInst::FCMP_OLT, V1, V2));
       if (R && !R->isZero()) 
Constant *llvm::ConstantFoldCompareInstruction(unsigned short pred,
   if (pred == FCmpInst::FCMP_FALSE)
     return Constant::getNullValue(ResultTy);
 
   if (pred == FCmpInst::FCMP_TRUE)
     return Constant::getAllOnesValue(ResultTy);
 
   // Handle some degenerate cases first
   if (isa<UndefValue>(C1) || isa<UndefValue>(C2)) {
+    CmpInst::Predicate Predicate = CmpInst::Predicate(pred);
     // For EQ and NE, we can always pick a value for the undef to make the
     // predicate pass or fail, so we can return undef.
-    // Also, if both operands are undef, we can return undef.
-    if (ICmpInst::isEquality(ICmpInst::Predicate(pred)) ||
-        (isa<UndefValue>(C1) && isa<UndefValue>(C2)))
+    // Also, if both operands are undef, we can return undef for int comparison.
+    if (ICmpInst::isEquality(Predicate) ||
+        (ICmpInst::isIntPredicate(Predicate) && isa<UndefValue>(C1) &&
+         isa<UndefValue>(C2)))
       return UndefValue::get(ResultTy);
-    // Otherwise, pick the same value as the non-undef operand, and fold
-    // it to true or false.
+
+    // Otherwise, for integer compare, pick the same value as the non-undef
+    // operand, and fold it to true or false.
+    if (CmpInst::isIntPredicate(Predicate))
       return ConstantInt::get(ResultTy, CmpInst::isTrueWhenEqual(pred));
+
+    // Choosing NaN for the undef will always make unordered comparison succeed
+    // and ordered comparison fails.
+    return ConstantInt::get(ResultTy, CmpInst::isUnordered(Predicate));
   }
 
   // icmp eq/ne(null,GV) -> false/true
   if (C1->isNullValue()) {
     if (const GlobalValue *GV = dyn_cast<GlobalValue>(C2))
       // Don't try to evaluate aliases.  External weak GV can be null.
       if (!isa<GlobalAlias>(GV) && !GV->hasExternalWeakLinkage()) {
         if (pred == ICmpInst::ICMP_EQ)
for (unsigned i = 0, e = C1->getType()->getVectorNumElements(); i != e;++i){
         ConstantExpr::getExtractElement(C2, ConstantInt::get(Ty, i));
       
       ResElts.push_back(ConstantExpr::getCompare(pred, C1E, C2E));
     }
     
     return ConstantVector::get(ResElts);
   }
 
-  if (C1->getType()->isFloatingPointTy()) {
+  if (C1->getType()->isFloatingPointTy() &&
+      // Only call evaluateFCmpRelation is we have a constant expr to avoid
+      // infinite recursive loop
+      (isa<ConstantExpr>(C1) || isa<ConstantExpr>(C2))) {
     int Result = -1;  // -1 = unknown, 0 = known false, 1 = known true.
     switch (evaluateFCmpRelation(C1, C2)) {
     default: llvm_unreachable("Unknown relation!");
     case FCmpInst::FCMP_UNO:
     case FCmpInst::FCMP_ORD:
     case FCmpInst::FCMP_UEQ:
     case FCmpInst::FCMP_UNE:
     case FCmpInst::FCMP_ULT:

test/Transforms/InstCombine/fcmp.ll

 
 ; 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
 }
+
+; Can fold fcmp with undef on one side by choosing NaN for the undef
+define i32 @test18_undef_unordered(float %a) nounwind {
+; CHECK-LABEL: @test18_undef_unordered
+; CHECK: ret i32 1
+  %cmp = fcmp ueq float %a, undef
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+}
+; Can fold fcmp with undef on one side by choosing NaN for the undef
+define i32 @test18_undef_ordered(float %a) nounwind {
+; CHECK-LABEL: @test18_undef_ordered
+; CHECK: ret i32 0
+  %cmp = fcmp oeq float %a, undef
+  %conv = zext i1 %cmp to i32
+  ret i32 %conv
+}
+
+; Can fold fcmp with undef on both side
+;   fcmp u_pred undef, undef -> true
+;   fcmp o_pred undef, undef -> false
+; because whatever you choose for the first undef
+; you can choose NaN for the other undef
+define i1 @test19_undef_unordered() nounwind {
+; CHECK-LABEL: @test19_undef
+; CHECK: ret i1 true
+  %cmp = fcmp ueq float undef, undef
+  ret i1 %cmp
+}
+define i1 @test19_undef_ordered() nounwind {
+; CHECK-LABEL: @test19_undef
+; CHECK: ret i1 false
+  %cmp = fcmp oeq float undef, undef
+  ret i1 %cmp
+}