ValueTracking: Replace CannotBeNegativeZero

[arsenm]

Summary

This is now just a wrapper around computeKnownFPClass.

[foad]

Code looks fine to me but you need to fix either the tests or the comments on them.

[arsenm]

In D148858#4286616, @foad wrote:

Code looks fine to me but you need to fix either the tests or the comments on them.

I think I’ll just defer the handling of constrained_sqrt and handle it in that patch

[kpn]

Are we certain that a constrained sqrt() folded away will be as accurate as not folded when the rounding mode is different? There's a lot of permutations of library calls and instructions that constrained sqrt() can be lowered to, and I'm worried that the results will be surprisingly bad with different rounding modes. If we don't fold then the end user can deal with library issues themselves at least, and hopefully square root instructions will be on CPUs that properly handle the different rounding modes.

I'm pretty sure I wrote those comments, and I think not folding due to the rounding mode is conservatively correct. I really don't think changing this behavior belongs in a patch titled "ValueTracking: Replace CannotBeNegativeZero". Changing sqrt() folding should be a different patch.

And if constrained sqrt is folded away then the "Negative test:" comments should be removed. With the fold the comments are incorrect.

[arsenm]

In D148858#4287981, @kpn wrote:

Are we certain that a constrained sqrt() folded away will be as accurate as not folded when the rounding mode is different?

I don't see how rounding can come into this. The only way to produce a -0 is a -0 source, where no rounding is involved. The exception would be for denormal flushing which isn't quite the same as rounding

[arsenm]

Rebase

[arsenm]

ping

[foad]

Code LGTM.

It
mishandles PreserveSign denormal inputs but that's a preexisting
issue.

Mishandles? Isn't it up to the caller to worry about that?

strictfp tests are improvements. The test comment suggests these
folds should not happen, but I think are correct.

Remove this paragraph.

[arsenm]

0f4eb557e87912afa412b1b0299d10eb7443410b

llvm/include/llvm/Analysis/ValueTracking.h

bool isKnownNeverZero() const {
     return isKnownNever(fcZero);
   }
 
   /// Return true if it's known this can never be a literal positive zero.
   bool isKnownNeverPosZero() const {
     return isKnownNever(fcPosZero);
   }
 
-  /// Return true if it's known this can never be a literal negative zero.
+  /// Return true if it's known this can never be a negative zero. This means a
+  /// literal -0 and does not include denormal inputs implicitly treated as -0.
   bool isKnownNeverNegZero() const {
     return isKnownNever(fcNegZero);
   }
 
   /// Return true if it's know this can never be interpreted as a zero. This
   /// extends isKnownNeverZero to cover the case where the assumed
   /// floating-point mode for the function interprets denormals as zero.
   bool isKnownNeverLogicalZero(const Function &F, Type *Ty) const;

 KnownFPClass computeKnownFPClass(
     const Value *V, const DataLayout &DL,
     FPClassTest InterestedClasses = fcAllFlags, unsigned Depth = 0,
     const TargetLibraryInfo *TLI = nullptr, AssumptionCache *AC = nullptr,
     const Instruction *CxtI = nullptr, const DominatorTree *DT = nullptr,
     bool UseInstrInfo = true);
 
 /// Return true if we can prove that the specified FP value is never equal to
-/// -0.0.
-bool CannotBeNegativeZero(const Value *V, const TargetLibraryInfo *TLI,
-                          unsigned Depth = 0);
-
+/// -0.0. Users should use caution when considering PreserveSign
+/// denormal-fp-math.
+inline bool cannotBeNegativeZero(const Value *V, const DataLayout &DL,
+                                 const TargetLibraryInfo *TLI = nullptr,
+                                 unsigned Depth = 0,
+                                 AssumptionCache *AC = nullptr,
+                                 const Instruction *CtxI = nullptr,
+                                 const DominatorTree *DT = nullptr,
+                                 bool UseInstrInfo = true) {
+  KnownFPClass Known = computeKnownFPClass(V, DL, fcNegZero, Depth, TLI, AC,
+                                           CtxI, DT, UseInstrInfo);
+  return Known.isKnownNeverNegZero();
+}
 
 bool CannotBeOrderedLessThanZero(const Value *V, const DataLayout &DL,
                                  const TargetLibraryInfo *TLI);
 
 /// Return true if we can prove that the specified FP value is either NaN or
 /// never less than -0.0.
 ///
 ///      NaN --> true

llvm/lib/Analysis/InstructionSimplify.cpp

if (canIgnoreSNaN(ExBehavior, FMF) &&
       (!canRoundingModeBe(Rounding, RoundingMode::TowardNegative) ||
        FMF.noSignedZeros()))
     if (match(Op1, m_NegZeroFP()))
       return Op0;
 
   // fadd X, 0 ==> X, when we know X is not -0
   if (canIgnoreSNaN(ExBehavior, FMF))
     if (match(Op1, m_PosZeroFP()) &&
-        (FMF.noSignedZeros() || CannotBeNegativeZero(Op0, Q.TLI)))
+        (FMF.noSignedZeros() || cannotBeNegativeZero(Op0, Q.DL, Q.TLI)))
       return Op0;
 
   if (!isDefaultFPEnvironment(ExBehavior, Rounding))
     return nullptr;
 
   if (FMF.noNaNs()) {
     // With nnan: X + {+/-}Inf --> {+/-}Inf
     if (match(Op1, m_Inf()))
if (canIgnoreSNaN(ExBehavior, FMF) &&
       (!canRoundingModeBe(Rounding, RoundingMode::TowardNegative) ||
        FMF.noSignedZeros()))
     if (match(Op1, m_PosZeroFP()))
       return Op0;
 
   // fsub X, -0 ==> X, when we know X is not -0
   if (canIgnoreSNaN(ExBehavior, FMF))
     if (match(Op1, m_NegZeroFP()) &&
-        (FMF.noSignedZeros() || CannotBeNegativeZero(Op0, Q.TLI)))
+        (FMF.noSignedZeros() || cannotBeNegativeZero(Op0, Q.DL, Q.TLI)))
       return Op0;
 
   // fsub -0.0, (fsub -0.0, X) ==> X
   // fsub -0.0, (fneg X) ==> X
   Value *X;
   if (canIgnoreSNaN(ExBehavior, FMF))
     if (match(Op0, m_NegZeroFP()) && match(Op1, m_FNeg(m_Value(X))))
       return X;

llvm/lib/Analysis/ValueTracking.cpp

Intrinsic::ID llvm::getIntrinsicForCallSite(const CallBase &CB,
   case LibFunc_sqrtf:
   case LibFunc_sqrtl:
     return Intrinsic::sqrt;
   }
 
   return Intrinsic::not_intrinsic;
 }
 
-/// Return true if we can prove that the specified FP value is never equal to
-/// -0.0.
-/// NOTE: Do not check 'nsz' here because that fast-math-flag does not guarantee
-///       that a value is not -0.0. It only guarantees that -0.0 may be treated
-///       the same as +0.0 in floating-point ops.
-bool llvm::CannotBeNegativeZero(const Value *V, const TargetLibraryInfo *TLI,
-                                unsigned Depth) {
-  if (auto *CFP = dyn_cast<ConstantFP>(V))
-    return !CFP->getValueAPF().isNegZero();
-
-  if (Depth == MaxAnalysisRecursionDepth)
-    return false;
-
-  auto *Op = dyn_cast<Operator>(V);
-  if (!Op)
-    return false;
-
-  // (fadd x, 0.0) is guaranteed to return +0.0, not -0.0.
-  if (match(Op, m_FAdd(m_Value(), m_PosZeroFP())))
-    return true;
-
-  // sitofp and uitofp turn into +0.0 for zero.
-  if (isa<SIToFPInst>(Op) || isa<UIToFPInst>(Op))
-    return true;
-
-  if (auto *Call = dyn_cast<CallInst>(Op)) {
-    Intrinsic::ID IID = getIntrinsicForCallSite(*Call, TLI);
-    switch (IID) {
-    default:
-      break;
-    case Intrinsic::sqrt:
-    case Intrinsic::experimental_constrained_sqrt:
-      // sqrt(-0.0) = -0.0, no other negative results are possible.
-      // FIXME: Account for denormal-fp-math=preserve-sign denormal inputs
-    case Intrinsic::canonicalize:
-      return CannotBeNegativeZero(Call->getArgOperand(0), TLI, Depth + 1);
-    // fabs(x) != -0.0
-    case Intrinsic::fabs:
-      return true;
-    // sitofp and uitofp turn into +0.0 for zero.
-    case Intrinsic::experimental_constrained_sitofp:
-    case Intrinsic::experimental_constrained_uitofp:
-      return true;
-    }
-  }
-
-  return false;
-}
-
 /// 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 DataLayout &DL,
                                             const TargetLibraryInfo *TLI,
                                             bool SignBitOnly, unsigned Depth) {
   // TODO: This function does not do the right thing when SignBitOnly is true
case Instruction::Call:
     case Intrinsic::copysign:
       // Only the sign operand matters.
       return cannotBeOrderedLessThanZeroImpl(I->getOperand(1), DL, TLI, true,
                                              Depth + 1);
     case Intrinsic::sqrt:
       // sqrt(x) is always >= -0 or NaN.  Moreover, sqrt(x) == -0 iff x == -0.
       if (!SignBitOnly)
         return true;
-      return CI->hasNoNaNs() && (CI->hasNoSignedZeros() ||
-                                 CannotBeNegativeZero(CI->getOperand(0), TLI));
+      return CI->hasNoNaNs() &&
+             (CI->hasNoSignedZeros() ||
+              cannotBeNegativeZero(CI->getOperand(0), DL, 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
static void computeKnownFPClassForFPTrunc(const Operator *Op,
   if (KnownSrc.cannotBeOrderedLessThanZero())
     Known.knownNot(KnownFPClass::OrderedLessThanZeroMask);
 
   Known.propagateNaN(KnownSrc, true);
 
   // Infinity needs a range check.
 }
 
-// TODO: Merge implementations of CannotBeNegativeZero,
-// cannotBeOrderedLessThanZero into here.
+// TODO: Merge implementation of cannotBeOrderedLessThanZero into here.
 void computeKnownFPClass(const Value *V, const APInt &DemandedElts,
                          FPClassTest InterestedClasses, KnownFPClass &Known,
                          unsigned Depth, const SimplifyQuery &Q) {
   assert(Known.isUnknown() && "should not be called with known information");
 
   if (!DemandedElts) {
     // No demanded elts, better to assume we don't know anything.
     Known.resetAll();

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

Instruction *InstCombinerImpl::visitFSub(BinaryOperator &I) {
 
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
   // If Op0 is not -0.0 or we can ignore -0.0: Z - (X - Y) --> Z + (Y - X)
   // Canonicalize to fadd to make analysis easier.
   // This can also help codegen because fadd is commutative.
   // Note that if this fsub was really an fneg, the fadd with -0.0 will get
   // killed later. We still limit that particular transform with 'hasOneUse'
   // because an fneg is assumed better/cheaper than a generic fsub.
-  if (I.hasNoSignedZeros() || CannotBeNegativeZero(Op0, SQ.TLI)) {
+  if (I.hasNoSignedZeros() || cannotBeNegativeZero(Op0, SQ.DL, SQ.TLI)) {
     if (match(Op1, m_OneUse(m_FSub(m_Value(X), m_Value(Y))))) {
       Value *NewSub = Builder.CreateFSubFMF(Y, X, &I);
       return BinaryOperator::CreateFAddFMF(Op0, NewSub, &I);
     }
   }
 
   // (-X) - Op1 --> -(X + Op1)
   if (I.hasNoSignedZeros() && !isa<ConstantExpr>(Op0) &&

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

static Instruction *foldSelectBinOpIdentity(SelectInst &Sel,
   if (!BO->isCommutative() && !match(BO, m_BinOp(m_Value(Y), m_Specific(X))))
     return nullptr;
   if (!match(BO, m_c_BinOp(m_Value(Y), m_Specific(X))))
     return nullptr;
 
   // +0.0 compares equal to -0.0, and so it does not behave as required for this
   // transform. Bail out if we can not exclude that possibility.
   if (isa<FPMathOperator>(BO))
-    if (!BO->hasNoSignedZeros() && !CannotBeNegativeZero(Y, &TLI))
+    if (!BO->hasNoSignedZeros() &&
+        !cannotBeNegativeZero(Y, IC.getDataLayout(), &TLI))
       return nullptr;
 
   // BO = binop Y, X
   // S = { select (cmp eq X, C), BO, ? } or { select (cmp ne X, C), ?, BO }
   // =>
   // S = { select (cmp eq X, C),  Y, ? } or { select (cmp ne X, C), ?,  Y }
   return IC.replaceOperand(Sel, IsEq ? 1 : 2, Y);
 }