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

[Justin Lebar via Phabricator via llvm-commits]

jlebar created this revision.

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.


https://reviews.llvm.org/D28927

Files:
  llvm/lib/Analysis/ValueTracking.cpp


Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -2602,6 +2602,12 @@
                                             const TargetLibraryInfo *TLI,
                                             bool SignBitOnly,
                                             unsigned Depth) {
+  // TODO: We are not consistent about how we handle the sign bit of propagated
+  // NaNs.  IEEE 754 section 6.3 says that the sign bit of a NaN returned by an
+  // operation is unspecified.  In some but not all places in this function, we
+  // assume the sign bit is 0.  We should have a principled approach here one
+  // way or another.
+
   if (const ConstantFP *CFP = dyn_cast<ConstantFP>(V)) {
     return !CFP->getValueAPF().isNegative() ||
            (!SignBitOnly && CFP->getValueAPF().isZero());
@@ -2670,8 +2676,23 @@
         if (CI->getBitWidth() <= 64 && CI->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.
+      //
+      // It's therefore sufficient to prove that x is positive (or NaN) but we
+      // must exclude x == -0 even if we normally would return true for -0.
+      // Fixing this without regressing existing cases that we correctly
+      // optimize probably requires adopting a principled approach to handling
+      // the sign bits of NaNs; see the TODO at the top of this function.
       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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28927.85062.patch
Type: text/x-patch
Size: 2153 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170120/444bad1a/attachment.bin>