[ValueTracking] fix maxnum miscompile for cannotBeOrderedLessThanZero (PR37776)

[spatel]

Summary

The definition of maxnum may still be up for discussion on llvm-dev, but I think we need to fix this miscompile regardless of that.

This adds the NAN checks suggested by @t.p.northover in PR37776:
https://bugs.llvm.org/show_bug.cgi?id=37776

If both operands to maxnum are NAN, that should get constant folded, so I don't think we have to handle that case.

Copying from the bug report:

Currently, we have this for "when is cannotBeOrderedLessThanZero (mustBePositiveOrNaN) true for maxnum":
               L
        -------------------
        | Pos | Neg | NaN |
   ------------------------
   |Pos |  x  |  x  |  x  |
   ------------------------
 R |Neg |  x  |     |  x  |
   ------------------------
   |NaN |  x  |  x  |  x  |
   ------------------------


The cases with (Neg & NaN) are wrong. We should have:

                L
        -------------------
        | Pos | Neg | NaN |
   ------------------------
   |Pos |  x  |  x  |  x  |
   ------------------------
 R |Neg |  x  |     |     |
   ------------------------
   |NaN |  x  |     |  x  |
   ------------------------

[arsenm]

The nan, nan case may be constant folded, but I think this still needs to handle it correctly. A pass could still ask this on a call that hasn't been folded yet (although I guess this can't really be relied on for correctness anyway). The nan && nan case at least needs a comment

[spatel]

In D50081#1183166, @arsenm wrote:

The nan, nan case may be constant folded, but I think this still needs to handle it correctly. A pass could still ask this on a call that hasn't been folded yet (although I guess this can't really be relied on for correctness anyway). The nan && nan case at least needs a comment

It's conservatively correct to return false, so I don't think maxnum is worth commenting specifically. For example, above here, we don't check for NaN inputs to the regular FP binops even though those would also return NaN (true). Should I add a general comment about that or leave it as-is?

[arsenm]

LGTM

[xbolva00]

Maybe this fix should be merged into 7.0 too?

@hans

[hans]

In D50081#1185447, @xbolva00 wrote:

Maybe this fix be should merged into 7.0 too?

@hans

Sounds good to me, but it needs to be committed first obviously :-)

[hans]

In D50081#1185480, @hans wrote:

In D50081#1185447, @xbolva00 wrote:

Maybe this fix be should merged into 7.0 too?

@hans

Sounds good to me, but it needs to be committed first obviously :-)

Merged in r339234.

[spatel]

In D50081#1192072, @hans wrote:

In D50081#1185480, @hans wrote:

In D50081#1185447, @xbolva00 wrote:

Maybe this fix be should merged into 7.0 too?

@hans

Sounds good to me, but it needs to be committed first obviously :-)

Merged in r339234.

Thanks!

lib/Analysis/ValueTracking.cpp

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 cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI, SignBitOnly,
-                                             Depth + 1) ||
-             cannotBeOrderedLessThanZeroImpl(I->getOperand(1), TLI, SignBitOnly,
-                                             Depth + 1);
+      return (isKnownNeverNaN(I->getOperand(0)) &&
+              cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI,
+                                              SignBitOnly, Depth + 1)) ||
+             (isKnownNeverNaN(I->getOperand(1)) &&
+              cannotBeOrderedLessThanZeroImpl(I->getOperand(1), TLI,
+                                              SignBitOnly, Depth + 1));
+
     case Intrinsic::minnum:
       return cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI, SignBitOnly,
                                              Depth + 1) &&
              cannotBeOrderedLessThanZeroImpl(I->getOperand(1), TLI, SignBitOnly,
                                              Depth + 1);
     case Intrinsic::exp:
     case Intrinsic::exp2:
     case Intrinsic::fabs:

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

 ;
   %a = call float @llvm.exp.f32(float %0)
   %b = call float @llvm.fabs.f32(float %1)
   %c = call float @llvm.minnum.f32(float %a, float %b)
   %uge = fcmp uge float %c, 0.000000e+00
   ret i1 %uge
 }
 
-; FIXME: This is wrong.
 ; PR37776: https://bugs.llvm.org/show_bug.cgi?id=37776
 ; exp() may return nan, leaving %1 as the unknown result, so we can't simplify.
 
 define i1 @orderedLessZeroMaxNum(float, float) {
 ; CHECK-LABEL: @orderedLessZeroMaxNum(
-; CHECK-NEXT:    ret i1 true
+; CHECK-NEXT:    [[A:%.*]] = call float @llvm.exp.f32(float [[TMP0:%.*]])
+; CHECK-NEXT:    [[B:%.*]] = call float @llvm.maxnum.f32(float [[A]], float [[TMP1:%.*]])
+; CHECK-NEXT:    [[UGE:%.*]] = fcmp uge float [[B]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[UGE]]
 ;
   %a = call float @llvm.exp.f32(float %0)
   %b = call float @llvm.maxnum.f32(float %a, float %1)
   %uge = fcmp uge float %b, 0.000000e+00
   ret i1 %uge
 }
 
 define i1 @known_positive_olt_with_negative_constant(double %a) {