[GVN} don't propagate equality comparisons of FP zero (PR22376)

[spatel]

Summary

In http://reviews.llvm.org/D6911, we allowed GVN to propagate FP equalities to allow some simple value range optimizations. But that introduced a bug when comparing to -0.0 or 0.0: these compare equal even though they are not bitwise identical.

This patch disallows propagating zero constants in equality comparisons. As shown in http://llvm.org/bugs/show_bug.cgi?id=22376, we can miscompile with the current optimization.

I also checked to make sure that NaN comparisons are handled, and they always appear to be correctly optimized by SimplifyInstruction.

[ArchDRobison]

The patch looks right to me. Informally, it's saying zero is not a constant :-)

[rnk]

Don't we have FP math flags for this? See FastMathFlags::noSignedZeros(). Looks like we don't have those on fcmp instrs, though.

We should consider doing this folding if signed zeros are disabled.

[spatel]

In D7257#115344, @rnk wrote:

Don't we have FP math flags for this? See FastMathFlags::noSignedZeros(). Looks like we don't have those on fcmp instrs, though.

We should consider doing this folding if signed zeros are disabled.

I thought about making this conditional on the IR-level FMF, but as you noted, they don't apply to fcmp (or intrinsics; see http://llvm.org/bugs/show_bug.cgi?id=21290 ). So until we get FMF fixed, we'd have to rely on a function-level attribute (unsafe-fp-math I think; I don't see an NSZ equivalent).

Ok, if I just put a 'ToDo' comment in this patch for this?

[hfinkel]

In D7257#115372, @spatel wrote:

In D7257#115344, @rnk wrote:

Don't we have FP math flags for this? See FastMathFlags::noSignedZeros(). Looks like we don't have those on fcmp instrs, though.

We should consider doing this folding if signed zeros are disabled.

I thought about making this conditional on the IR-level FMF, but as you noted, they don't apply to fcmp (or intrinsics; see http://llvm.org/bugs/show_bug.cgi?id=21290 ). So until we get FMF fixed, we'd have to rely on a function-level attribute (unsafe-fp-math I think; I don't see an NSZ equivalent).

Ok, if I just put a 'ToDo' comment in this patch for this?

Yes, add a FIXME for this. LGTM.

[spatel]

Thanks all. Added a FIXME comment and a test case for the -0.0 + fcmp une half.
Checked in at r227491.

llvm/trunk/lib/Transforms/Scalar/GVN.cpp

if (CmpInst *Cmp = dyn_cast<CmpInst>(LHS)) {
       // If "A == B" is known true, or "A != B" is known false, then replace
       // A with B everywhere in the scope.
       if ((isKnownTrue && Cmp->getPredicate() == CmpInst::ICMP_EQ) ||
           (isKnownFalse && Cmp->getPredicate() == CmpInst::ICMP_NE))
         Worklist.push_back(std::make_pair(Op0, Op1));
 
       // Handle the floating point versions of equality comparisons too.
       if ((isKnownTrue && Cmp->getPredicate() == CmpInst::FCMP_OEQ) ||
-          (isKnownFalse && Cmp->getPredicate() == CmpInst::FCMP_UNE))
+          (isKnownFalse && Cmp->getPredicate() == CmpInst::FCMP_UNE)) {
+        // Floating point -0.0 and 0.0 compare equal, so we can't
+        // propagate a constant based on that comparison.
+        // FIXME: We should do this optimization if 'no signed zeros' is
+        // applicable via an instruction-level fast-math-flag or some other
+        // indicator that relaxed FP semantics are being used.
+        if (!isa<ConstantFP>(Op1) || !cast<ConstantFP>(Op1)->isZero())
           Worklist.push_back(std::make_pair(Op0, Op1));
+      }
  
       // If "A >= B" is known true, replace "A < B" with false everywhere.
       CmpInst::Predicate NotPred = Cmp->getInversePredicate();
       Constant *NotVal = ConstantInt::get(Cmp->getType(), isKnownFalse);
       // Since we don't have the instruction "A < B" immediately to hand, work
       // out the value number that it would have and use that to find an
       // appropriate instruction (if any).
       uint32_t NextNum = VN.getNextUnusedValueNumber();
       uint32_t Num = VN.lookup_or_add_cmp(Cmp->getOpcode(), NotPred, Op0, Op1);

llvm/trunk/test/Transforms/GVN/edge.ll

entry:
   %cmp = fcmp oeq double %y, 2.0
   br i1 %cmp, label %if, label %return
 
 if:
   %div = fdiv double %x, %y
   br label %return
 
 return:
-  %retval.0 = phi double [ %div, %if ], [ %x, %entry ]
-  ret double %retval.0
+  %retval = phi double [ %div, %if ], [ %x, %entry ]
+  ret double %retval
 
 ; CHECK-LABEL: define double @fcmp_oeq(
-; CHECK: %div = fdiv double %x, 2.000000e+00
+; CHECK: %div = fdiv double %x, 2.0
 }
 
 define double @fcmp_une(double %x, double %y) {
 entry:
   %cmp = fcmp une double %y, 2.0
   br i1 %cmp, label %return, label %else
 
 else:
   %div = fdiv double %x, %y
   br label %return
 
 return:
-  %retval.0 = phi double [ %div, %else ], [ %x, %entry ]
-  ret double %retval.0
+  %retval = phi double [ %div, %else ], [ %x, %entry ]
+  ret double %retval
 
 ; CHECK-LABEL: define double @fcmp_une(
-; CHECK: %div = fdiv double %x, 2.000000e+00
+; CHECK: %div = fdiv double %x, 2.0
 }
 
+; PR22376 - We can't propagate zero constants because -0.0 
+; compares equal to 0.0. If %y is -0.0 in this test case,
+; we would produce the wrong sign on the infinity return value.
+define double @fcmp_oeq_zero(double %x, double %y) {
+entry:
+  %cmp = fcmp oeq double %y, 0.0
+  br i1 %cmp, label %if, label %return
+
+if:
+  %div = fdiv double %x, %y
+  br label %return
+
+return:
+  %retval = phi double [ %div, %if ], [ %x, %entry ]
+  ret double %retval
+
+; CHECK-LABEL: define double @fcmp_oeq_zero(
+; CHECK: %div = fdiv double %x, %y
+}
+
+define double @fcmp_une_zero(double %x, double %y) {
+entry:
+  %cmp = fcmp une double %y, -0.0
+  br i1 %cmp, label %return, label %else
+
+else:
+  %div = fdiv double %x, %y
+  br label %return
+
+return:
+  %retval = phi double [ %div, %else ], [ %x, %entry ]
+  ret double %retval
+
+; CHECK-LABEL: define double @fcmp_une_zero(
+; CHECK: %div = fdiv double %x, %y
+}