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

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: efriedma, cameron.mcinally, arsenm.
Herald added subscribers: hiraditya, wdng, mcrosier.
Herald added a project: LLVM.

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).


https://reviews.llvm.org/D83601

Files:
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll


Index: llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll
===================================================================
--- llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll
+++ llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll
@@ -1334,10 +1334,13 @@
   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)
@@ -1348,7 +1351,8 @@
 ; 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)
@@ -1361,7 +1365,8 @@
 ; 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)
@@ -1375,7 +1380,8 @@
 ; 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)
Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -3368,13 +3368,20 @@
     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: {
+      auto isPositiveNum = [&](Value *V) {
+        return isKnownNeverNaN(V, TLI) &&
+               cannotBeOrderedLessThanZeroImpl(V, TLI, SignBitOnly, Depth + 1);
+      };
+
+      // This is tricky because the result of maxnum(+0.0, -0.0) is unspecified.
+      // TODO: This could be improved. If we had a "CannotBePositiveZero", then
+      //       that plus isPositiveNum is enough to say maxnum returns a
+      //       positive value.
+      Value *V0 = I->getOperand(0), *V1 = I->getOperand(1);
+      return (isPositiveNum(V0) && CannotBeNegativeZero(V1, TLI, Depth + 1)) ||
+             (isPositiveNum(V1) && CannotBeNegativeZero(V0, TLI, Depth + 1));
+    }
 
     case Intrinsic::maximum:
       return cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI, SignBitOnly,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D83601.277183.patch
Type: text/x-patch
Size: 3899 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200711/8e308200/attachment.bin>