[ValueTracking] fix bug in maxnum case of cannotBeOrderedLessThanZeroImpl (PR46627)

[spatel]

Summary

A miscompile with -0.0 is shown in:
https://bugs.llvm.org/show_bug.cgi?id=46627

This is because maxnum(-0.0, +0.0) does not specify a fixed result:
http://llvm.org/docs/LangRef.html#llvm-maxnum-intrinsic

So we need to tighten the constraints for when it is ok to say the result of maxnum is positive (including +0.0).

[efriedma]

llvm/lib/Analysis/ValueTracking.cpp

3383

Can you just pass false for "SignBitOnly", instead of doing a bunch of extra calls?

[efriedma]

llvm/lib/Analysis/ValueTracking.cpp

3383

Err, nevermind, that doesn't work; I need more coffee this morning.

[efriedma]

llvm/lib/Analysis/ValueTracking.cpp

3383

Hopefully correct this time:

If SignBitOnly is false, the current code is correct, I think. The sign bit of zero doesn't matter.

If SignBitOnly is true, there are two possible proof methods:

1. Neither operand has the sign bit set.
2. Either operand is strictly greater than zero.

[spatel]

Patch updated:

1. Salvage a bit more of the existing functionality by differentiating based on "SignBitOnly" (shown with the unchanged test code comments).
2. Add another test (copysign) to show a miscompile.

[efriedma]

LGTM

llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll

1393

"the sign of the result"

llvm/lib/Analysis/ValueTracking.cpp

case Instruction::ExtractElement:
     return cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI, SignBitOnly,
                                            Depth + 1);
   case Instruction::Call:
     const auto *CI = cast<CallInst>(I);
     Intrinsic::ID IID = getIntrinsicForCallSite(*CI, TLI);
     switch (IID) {
     default:
       break;
-    case Intrinsic::maxnum:
-      return (isKnownNeverNaN(I->getOperand(0), TLI) &&
-              cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI,
-                                              SignBitOnly, Depth + 1)) ||
-            (isKnownNeverNaN(I->getOperand(1), TLI) &&
-              cannotBeOrderedLessThanZeroImpl(I->getOperand(1), TLI,
-                                              SignBitOnly, Depth + 1));
+    case Intrinsic::maxnum: {
+      Value *V0 = I->getOperand(0), *V1 = I->getOperand(1);
+      auto isPositiveNum = [&](Value *V) {
+        if (SignBitOnly) {
+          // With SignBitOnly, this is tricky because the result of
+          // maxnum(+0.0, -0.0) is unspecified. Just check if the operand is
+          // a constant strictly greater than 0.0.
+          const APFloat *C;
+          return match(V, m_APFloat(C)) &&
+                 *C > APFloat::getZero(C->getSemantics());
+        }
+
+        // -0.0 compares equal to 0.0, so if this operand is at least -0.0,
+        // maxnum can't be ordered-less-than-zero.
+        return isKnownNeverNaN(V, TLI) &&
+               cannotBeOrderedLessThanZeroImpl(V, TLI, false, Depth + 1);
+      };
+
+      // TODO: This could be improved. We could also check that neither operand
+      //       has its sign bit set (and at least 1 is not-NAN?).
+      return isPositiveNum(V0) || isPositiveNum(V1);
+    }
 
     case Intrinsic::maximum:
       return cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI, SignBitOnly,
                                              Depth + 1) ||
              cannotBeOrderedLessThanZeroImpl(I->getOperand(1), TLI, SignBitOnly,
                                              Depth + 1);
     case Intrinsic::minnum:
     case Intrinsic::minimum:

llvm/test/Transforms/InstCombine/copysign.ll

 ; CHECK-NEXT:    [[TMP1:%.*]] = call arcp <3 x double> @llvm.fabs.v3f64(<3 x double> [[X:%.*]])
 ; CHECK-NEXT:    ret <3 x double> [[TMP1]]
 ;
   %yf = uitofp <3 x i32> %y to <3 x double>
   %r = call arcp <3 x double> @llvm.copysign.v3f64(<3 x double> %x, <3 x double> %yf)
   ret <3 x double> %r
 }
 
-; FIXME: maxnum(-0.0, 0.0) can return -0.0.
+; maxnum(-0.0, 0.0) can return -0.0.
 
 define float @not_known_positive_sign_arg(float %x, float %y) {
 ; CHECK-LABEL: @not_known_positive_sign_arg(
-; CHECK-NEXT:    [[TMP1:%.*]] = call ninf float @llvm.fabs.f32(float [[Y:%.*]])
-; CHECK-NEXT:    ret float [[TMP1]]
+; CHECK-NEXT:    [[MAX:%.*]] = call float @llvm.maxnum.f32(float [[X:%.*]], float 0.000000e+00)
+; CHECK-NEXT:    [[R:%.*]] = call ninf float @llvm.copysign.f32(float [[Y:%.*]], float [[MAX]])
+; CHECK-NEXT:    ret float [[R]]
 ;
   %max = call float @llvm.maxnum.f32(float %x, float 0.0)
   %r = call ninf float @llvm.copysign.f32(float %y, float %max)
   ret float %r
 }
 
 ; The magnitude operand of the 1st copysign is irrelevant.
 ; copysign(x, copysign(y, z)) --> copysign(x, z)

llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll

 ; CHECK-NEXT:    [[R:%.*]] = fadd reassoc nsz float [[S]], [[Y]]
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %s = fsub float %y, %x
   %r = fadd reassoc nsz float %s, %y
   ret float %r
 }
 
+; PR46627 - https://bugs.llvm.org/show_bug.cgi?id=46627
+
 define float @maxnum_with_poszero_op(float %a) {
 ; CHECK-LABEL: @maxnum_with_poszero_op(
 ; CHECK-NEXT:    [[MAX:%.*]] = call float @llvm.maxnum.f32(float [[A:%.*]], float 0.000000e+00)
-; CHECK-NEXT:    ret float [[MAX]]
+; CHECK-NEXT:    [[FABS:%.*]] = call float @llvm.fabs.f32(float [[MAX]])
+; CHECK-NEXT:    ret float [[FABS]]
 ;
   %max = call float @llvm.maxnum.f32(float %a, float 0.0)
   %fabs = call float @llvm.fabs.f32(float %max)
   ret float %fabs
 }
 
 define float @maxnum_with_poszero_op_commute(float %a) {
 ; CHECK-LABEL: @maxnum_with_poszero_op_commute(
 ; CHECK-NEXT:    [[SQRT:%.*]] = call float @llvm.sqrt.f32(float [[A:%.*]])
 ; CHECK-NEXT:    [[MAX:%.*]] = call float @llvm.maxnum.f32(float 0.000000e+00, float [[SQRT]])
-; CHECK-NEXT:    ret float [[MAX]]
+; CHECK-NEXT:    [[FABS:%.*]] = call float @llvm.fabs.f32(float [[MAX]])
+; CHECK-NEXT:    ret float [[FABS]]
 ;
   %sqrt = call float @llvm.sqrt.f32(float %a)
   %max = call float @llvm.maxnum.f32(float 0.0, float %sqrt)
   %fabs = call float @llvm.fabs.f32(float %max)
   ret float %fabs
 }
 
 define float @maxnum_with_negzero_op(float %a) {
 ; CHECK-LABEL: @maxnum_with_negzero_op(
 ; CHECK-NEXT:    [[NNAN:%.*]] = call nnan float @llvm.sqrt.f32(float [[A:%.*]])
 ; CHECK-NEXT:    [[FABSA:%.*]] = call float @llvm.fabs.f32(float [[NNAN]])
 ; CHECK-NEXT:    [[MAX:%.*]] = call float @llvm.maxnum.f32(float -0.000000e+00, float [[FABSA]])
-; CHECK-NEXT:    ret float [[MAX]]
+; CHECK-NEXT:    [[FABS:%.*]] = call float @llvm.fabs.f32(float [[MAX]])
+; CHECK-NEXT:    ret float [[FABS]]
 ;
   %nnan = call nnan float @llvm.sqrt.f32(float %a)
   %fabsa = call float @llvm.fabs.f32(float %nnan)
   %max = call float @llvm.maxnum.f32(float -0.0, float %fabsa)
   %fabs = call float @llvm.fabs.f32(float %max)
   ret float %fabs
 }
 
 define float @maxnum_with_negzero_op_commute(float %a) {
 ; CHECK-LABEL: @maxnum_with_negzero_op_commute(
 ; CHECK-NEXT:    [[NNAN:%.*]] = call nnan float @llvm.sqrt.f32(float [[A:%.*]])
 ; CHECK-NEXT:    [[FABSA:%.*]] = call float @llvm.fabs.f32(float [[NNAN]])
 ; CHECK-NEXT:    [[MAX:%.*]] = call float @llvm.maxnum.f32(float [[FABSA]], float -0.000000e+00)
-; CHECK-NEXT:    ret float [[MAX]]
+; CHECK-NEXT:    [[FABS:%.*]] = call float @llvm.fabs.f32(float [[MAX]])
+; CHECK-NEXT:    ret float [[FABS]]
 ;
   %nnan = call nnan float @llvm.sqrt.f32(float %a)
   %fabsa = call float @llvm.fabs.f32(float %nnan)
   %max = call float @llvm.maxnum.f32(float %fabsa, float -0.0)
   %fabs = call float @llvm.fabs.f32(float %max)
   ret float %fabs
 }
 
+; If an operand is strictly greater than 0.0, we know the sign of the result of maxnum.
+
 define float @maxnum_with_pos_one_op(float %a) {
 ; CHECK-LABEL: @maxnum_with_pos_one_op(
 ; CHECK-NEXT:    [[MAX:%.*]] = call float @llvm.maxnum.f32(float [[A:%.*]], float 1.000000e+00)
 ; CHECK-NEXT:    ret float [[MAX]]
 ;
   %max = call float @llvm.maxnum.f32(float %a, float 1.0)
   %fabs = call float @llvm.fabs.f32(float %max)
   ret float %fabs
 }

llvm/test/Transforms/InstSimplify/floating-point-compare.ll

 ; CHECK-LABEL: @orderedLessZero_fdiv(
 ; CHECK-NEXT:    ret i1 true
 ;
   %d = fdiv float %x, %x
   %uge = fcmp uge float %d, 0.0
   ret i1 %uge
 }
 
+; If x == -0.0, maxnum can return -0.0, but that still compares equal to 0.0.
+
 define i1 @orderedLessZero_maxnum(float %x) {
 ; CHECK-LABEL: @orderedLessZero_maxnum(
 ; CHECK-NEXT:    ret i1 true
 ;
   %d = call float @llvm.maxnum.f32(float %x, float 0.0)
   %uge = fcmp uge float %d, 0.0
   ret i1 %uge
 }