[InstCombine] Pass the DominatorTree and AssumptionCache to a few calls to isKnownPositive, isKnownNegative, and isKnownNonZero

[craig.topper]

Summary

These calls aren't being passed the DominatorTree, and AssumptionCache or the correct CxtI instruction.

[spatel]

Can you concoct tests with 'llvm.assume' to show a difference?

I think that raises a question though: this change can make value tracking more effective, but it comes with non-zero compile-time cost if I understand computeKnownBits() correctly. Do we want to incur that overhead without a real-world motivation for the benefit?
cc'ing @dberlin @davide @hfinkel

Another question along this line: we call Simplify*Inst() for all instructions as the first step in every visit*() in InstCombine. In most cases, we don't pass a context instruction, but we do for visitICmpInst(). Is that difference intentional?

if (Value *V = SimplifyICmpInst(I.getPredicate(), Op0, Op1,
                                SQ.getWithInstruction(&I)))
  return replaceInstUsesWith(I, V);

vs.

if (Value *V = SimplifyShlInst(Op0, Op1, I.hasNoSignedWrap(),
                                I.hasNoUnsignedWrap(), SQ))
   return replaceInstUsesWith(I, V);

[craig.topper]

I was operating under the assumption that this was an accident. Most other places use the InstCombine specific wrappers around computeKnownBits so always pass the the dominatortree and assumption cache. If there's a reason for these places to be different then that should be commented.

[hfinkel]

In D33567#765886, @craig.topper wrote:

I was operating under the assumption that this was an accident. Most other places use the InstCombine specific wrappers around computeKnownBits so always pass the the dominatortree and assumption cache. If there's a reason for these places to be different then that should be commented.

I agree. Consistency is king in this regard, unless we have a specific (commented) reason to deviate. Otherwise, it makes it very difficult to reason about what the calls will do. When the original changes were made, all of these calls were swept in, and specific justification should not be required to correct oversights in new/other code.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp

static Instruction *foldCttzCtlz(IntrinsicInst &II, InstCombiner &IC) {
   if (PossibleZeros == DefiniteZeros) {
     auto *C = ConstantInt::get(IT, DefiniteZeros);
     return IC.replaceInstUsesWith(II, C);
   }
 
   // If the input to cttz/ctlz is known to be non-zero,
   // then change the 'ZeroIsUndef' parameter to 'true'
   // because we know the zero behavior can't affect the result.
-  if (Known.One != 0 || isKnownNonZero(Op0, IC.getDataLayout())) {
+  if (Known.One != 0 ||
+      isKnownNonZero(Op0, IC.getDataLayout(), 0, &IC.getAssumptionCache(), &II,
+                     &IC.getDominatorTree())) {
     if (!match(II.getArgOperand(1), m_One())) {
       II.setOperand(1, IC.Builder->getTrue());
       return ⅈ
     }
   }
 
   return nullptr;
 }

llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp

if (Pred == ICmpInst::ICMP_UGT &&
               Cmp, A, B, CI2, cast<ConstantInt>(Cmp.getOperand(1)), *this))
         return Res;
   }
 
   // (icmp sgt smin(PosA, B) 0) -> (icmp sgt B 0)
   if (*C == 0 && Pred == ICmpInst::ICMP_SGT) {
     SelectPatternResult SPR = matchSelectPattern(X, A, B);
     if (SPR.Flavor == SPF_SMIN) {
-      if (isKnownPositive(A, DL))
+      if (isKnownPositive(A, DL, 0, &AC, &Cmp, &DT))
         return new ICmpInst(Pred, B, Cmp.getOperand(1));
-      if (isKnownPositive(B, DL))
+      if (isKnownPositive(B, DL, 0, &AC, &Cmp, &DT))
         return new ICmpInst(Pred, A, Cmp.getOperand(1));
     }
   }
 
   // FIXME: Use m_APInt to allow folds for splat constants.
   ConstantInt *CI = dyn_cast<ConstantInt>(Cmp.getOperand(1));
   if (!CI)
     return nullptr;

llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp

if (Constant *CUI = dyn_cast<Constant>(Op1))
     if (Instruction *Res = FoldShiftByConstant(Op0, CUI, I))
       return Res;
 
   // (C1 shift (A add C2)) -> (C1 shift C2) shift A)
   // iff A and C2 are both positive.
   Value *A;
   Constant *C;
   if (match(Op0, m_Constant()) && match(Op1, m_Add(m_Value(A), m_Constant(C))))
-    if (isKnownNonNegative(A, DL) && isKnownNonNegative(C, DL))
+    if (isKnownNonNegative(A, DL, 0, &AC, &I, &DT) &&
+        isKnownNonNegative(C, DL, 0, &AC, &I, &DT))
       return BinaryOperator::Create(
           I.getOpcode(), Builder->CreateBinOp(I.getOpcode(), Op0, C), A);
 
   // X shift (A srem B) -> X shift (A and B-1) iff B is a power of 2.
   // Because shifts by negative values (which could occur if A were negative)
   // are undefined.
   const APInt *B;
   if (Op1->hasOneUse() && match(Op1, m_SRem(m_Value(A), m_Power2(B)))) {