[ValueTracking] Add comment that CannotBeOrderedLessThanZero does the wrong thing for powi.

[jlebar]

Summary

CannotBeOrderedLessThanZero(powi(x, exp)) returns true if
CannotBeOrderedLessThanZero(x). But powi(-0, exp) is negative if exp is
odd, so we actually want to return SignBitMustBeZero(x).

Except that also isn't right, because we want to return true if x is
NaN, even if x has a negative sign bit.

What we really need in order to fix this is a consistent approach in
this function to handling the sign bit of NaNs. Without this it's very
difficult to say what the correct behavior here is.

[efriedma]

https://reviews.llvm.org/D28926 provides perfectly clear definitions of what those functions are supposed to return; if something is wrong, just fix it (or file a bug). From your description, it sounds like we need to split "SignBitOnly" into "NegZeroSignBitSignificant" and "NanSignBitSignificant"?

[efriedma]

Oh, wait, I see, I really messed up my review of SignBitMustBeZero()... we return the wrong result for a ton of stuff. I'd still prefer a bug report with a testcase over a TODO comment.

[jlebar]

In D28927#651135, @efriedma wrote:

we return the wrong result for a ton of stuff.

Yes. :) Again it's not clear to me what it's really supposed to do.

I'd still prefer a bug report with a testcase over a TODO comment.

How about if you file a bug I'll edit the code to add a reference to it? I'd like to have a breadcrumb in the code indicating that this is wrong so that the next person who comes along and notices the breakage doesn't waste days on it like I did.

[efriedma]

Filed https://llvm.org/bugs/show_bug.cgi?id=31702 .

[jlebar]

Update comments. I made them shorter and added links to the bug.

I think this is an appropriate amount of brevity while still giving a reader
have some clue what's going on, but lmk what you think.

[hfinkel]

In D28927#651309, @jlebar wrote:

Update comments. I made them shorter and added links to the bug.

I think this is an appropriate amount of brevity while still giving a reader
have some clue what's going on, but lmk what you think.

Looks reasonable to me. @efriedma ?

[efriedma]

LGTM.

llvm/trunk/lib/Analysis/ValueTracking.cpp

 
 /// If \p SignBitOnly is true, test for a known 0 sign bit rather than a
 /// standard ordered compare. e.g. make -0.0 olt 0.0 be true because of the sign
 /// bit despite comparing equal.
 static bool cannotBeOrderedLessThanZeroImpl(const Value *V,
                                             const TargetLibraryInfo *TLI,
                                             bool SignBitOnly,
                                             unsigned Depth) {
+  // TODO: This function does not do the right thing when SignBitOnly is true
+  // and we're lowering to a hypothetical IEEE 754-compliant-but-evil platform
+  // which flips the sign bits of NaNs.  See
+  // https://llvm.org/bugs/show_bug.cgi?id=31702.
+
   if (const ConstantFP *CFP = dyn_cast<ConstantFP>(V)) {
     return !CFP->getValueAPF().isNegative() ||
            (!SignBitOnly && CFP->getValueAPF().isZero());
   }
 
   if (Depth == MaxDepth)
     return false; // Limit search depth.
 
case Intrinsic::sqrt:
                                  CannotBeNegativeZero(CI->getOperand(0), TLI));
 
     case Intrinsic::powi:
       if (ConstantInt *Exponent = dyn_cast<ConstantInt>(I->getOperand(1))) {
         // powi(x,n) is non-negative if n is even.
         if (Exponent->getBitWidth() <= 64 && Exponent->getSExtValue() % 2u == 0)
           return true;
       }
+      // TODO: This is not correct.  Given that exp is an integer, here are the
+      // ways that pow can return a negative value:
+      //
+      //   pow(x, exp)    --> negative if exp is odd and x is negative.
+      //   pow(-0, exp)   --> -inf if exp is negative odd.
+      //   pow(-0, exp)   --> -0 if exp is positive odd.
+      //   pow(-inf, exp) --> -0 if exp is negative odd.
+      //   pow(-inf, exp) --> -inf if exp is positive odd.
+      //
+      // Therefore, if !SignBitOnly, we can return true if x >= +0 or x is NaN,
+      // but we must return false if x == -0.  Unfortunately we do not currently
+      // have a way of expressing this constraint.  See details in
+      // https://llvm.org/bugs/show_bug.cgi?id=31702.
       return cannotBeOrderedLessThanZeroImpl(I->getOperand(0), TLI, SignBitOnly,
                                              Depth + 1);
+
     case Intrinsic::fma:
     case Intrinsic::fmuladd:
       // x*x+y is non-negative if y is non-negative.
       return I->getOperand(0) == I->getOperand(1) &&
              (!SignBitOnly || cast<FPMathOperator>(I)->hasNoNaNs()) &&
              cannotBeOrderedLessThanZeroImpl(I->getOperand(2), TLI, SignBitOnly,
                                              Depth + 1);
     }