[CVP] processCallSite returns wrong status

[ebrevnov]

Summary

Recently processMinMaxIntrinsic has been added and we started to observe a number of analysis get invalidated after CVP. The problem is CVP conservatively returns 'true' even if there were no modifications to IR. I found one more place besides processMinMaxIntrinsic which has the same problem. I think processMinMaxIntrinsic and similar should better have boolean return status to prevent similar issue reappear in future.

[lebedev.ri]

Claiming that changes were done where none were done is a bug too?

[ebrevnov]

In D100538#2690864, @lebedev.ri wrote:

Claiming that changes were done where none were done is a bug too?

It's not a functional bug. It's always legal to invalidate results of analysis but has negative impact on compile time.
PS: not sure if that answers your question though....

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp

static void setDeducedOverflowingFlags(Value *V, Instruction::BinaryOps Opcode,
   }
 }
 
 static bool processBinOp(BinaryOperator *BinOp, LazyValueInfo *LVI);
 
 // See if @llvm.abs argument is alays positive/negative, and simplify.
 // Notably, INT_MIN can belong to either range, regardless of the NSW,
 // because it is negation-invariant.
-static void processAbsIntrinsic(IntrinsicInst *II, LazyValueInfo *LVI) {
+static bool processAbsIntrinsic(IntrinsicInst *II, LazyValueInfo *LVI) {
   Value *X = II->getArgOperand(0);
   bool IsIntMinPoison = cast<ConstantInt>(II->getArgOperand(1))->isOne();
 
   Type *Ty = X->getType();
   Constant *IntMin =
       ConstantInt::get(Ty, APInt::getSignedMinValue(Ty->getScalarSizeInBits()));
   LazyValueInfo::Tristate Result;
 
   // Is X in [0, IntMin]?  NOTE: INT_MIN is fine!
   Result = LVI->getPredicateAt(CmpInst::Predicate::ICMP_ULE, X, IntMin, II,
                                /*UseBlockValue=*/true);
   if (Result == LazyValueInfo::True) {
     ++NumAbs;
     II->replaceAllUsesWith(X);
     II->eraseFromParent();
-    return;
+    return true;
   }
 
   // Is X in [IntMin, 0]?  NOTE: INT_MIN is fine!
   Constant *Zero = ConstantInt::getNullValue(Ty);
   Result = LVI->getPredicateAt(CmpInst::Predicate::ICMP_SLE, X, Zero, II,
                                /*UseBlockValue=*/true);
   assert(Result != LazyValueInfo::False && "Should have been handled already.");
 
   if (Result == LazyValueInfo::Unknown) {
     // Argument's range crosses zero.
+    bool Changed = false;
     if (!IsIntMinPoison) {
       // Can we at least tell that the argument is never INT_MIN?
       Result = LVI->getPredicateAt(CmpInst::Predicate::ICMP_NE, X, IntMin, II,
                                    /*UseBlockValue=*/true);
       if (Result == LazyValueInfo::True) {
         ++NumNSW;
         ++NumSubNSW;
         II->setArgOperand(1, ConstantInt::getTrue(II->getContext()));
+        Changed = true;
       }
     }
-    return;
+    return Changed;
   }
 
   IRBuilder<> B(II);
   Value *NegX = B.CreateNeg(X, II->getName(), /*HasNUW=*/false,
                             /*HasNSW=*/IsIntMinPoison);
   ++NumAbs;
   II->replaceAllUsesWith(NegX);
   II->eraseFromParent();
 
   // See if we can infer some no-wrap flags.
   if (auto *BO = dyn_cast<BinaryOperator>(NegX))
     processBinOp(BO, LVI);
+
+  return true;
 }
 
 // See if this min/max intrinsic always picks it's one specific operand.
-static void processMinMaxIntrinsic(MinMaxIntrinsic *MM, LazyValueInfo *LVI) {
+static bool processMinMaxIntrinsic(MinMaxIntrinsic *MM, LazyValueInfo *LVI) {
   CmpInst::Predicate Pred = CmpInst::getNonStrictPredicate(MM->getPredicate());
   LazyValueInfo::Tristate Result = LVI->getPredicateAt(
       Pred, MM->getLHS(), MM->getRHS(), MM, /*UseBlockValue=*/true);
   if (Result == LazyValueInfo::Unknown)
-    return;
+    return false;
 
   ++NumMinMax;
   MM->replaceAllUsesWith(MM->getOperand(!Result));
   MM->eraseFromParent();
+  return true;
 }
 
 // Rewrite this with.overflow intrinsic as non-overflowing.
-static void processOverflowIntrinsic(WithOverflowInst *WO, LazyValueInfo *LVI) {
+static bool processOverflowIntrinsic(WithOverflowInst *WO, LazyValueInfo *LVI) {
   IRBuilder<> B(WO);
   Instruction::BinaryOps Opcode = WO->getBinaryOp();
   bool NSW = WO->isSigned();
   bool NUW = !WO->isSigned();
 
   Value *NewOp =
       B.CreateBinOp(Opcode, WO->getLHS(), WO->getRHS(), WO->getName());
   setDeducedOverflowingFlags(NewOp, Opcode, NSW, NUW);
 
   StructType *ST = cast<StructType>(WO->getType());
   Constant *Struct = ConstantStruct::get(ST,
       { UndefValue::get(ST->getElementType(0)),
         ConstantInt::getFalse(ST->getElementType(1)) });
   Value *NewI = B.CreateInsertValue(Struct, NewOp, 0);
   WO->replaceAllUsesWith(NewI);
   WO->eraseFromParent();
   ++NumOverflows;
 
   // See if we can infer the other no-wrap too.
   if (auto *BO = dyn_cast<BinaryOperator>(NewOp))
     processBinOp(BO, LVI);
+
+  return true;
 }
 
-static void processSaturatingInst(SaturatingInst *SI, LazyValueInfo *LVI) {
+static bool processSaturatingInst(SaturatingInst *SI, LazyValueInfo *LVI) {
   Instruction::BinaryOps Opcode = SI->getBinaryOp();
   bool NSW = SI->isSigned();
   bool NUW = !SI->isSigned();
   BinaryOperator *BinOp = BinaryOperator::Create(
       Opcode, SI->getLHS(), SI->getRHS(), SI->getName(), SI);
   BinOp->setDebugLoc(SI->getDebugLoc());
   setDeducedOverflowingFlags(BinOp, Opcode, NSW, NUW);
 
   SI->replaceAllUsesWith(BinOp);
   SI->eraseFromParent();
   ++NumSaturating;
 
   // See if we can infer the other no-wrap too.
   if (auto *BO = dyn_cast<BinaryOperator>(BinOp))
     processBinOp(BO, LVI);
+
+  return true;
 }
 
 /// Infer nonnull attributes for the arguments at the specified callsite.
 static bool processCallSite(CallBase &CB, LazyValueInfo *LVI) {
 
   if (CB.getIntrinsicID() == Intrinsic::abs) {
-    processAbsIntrinsic(&cast<IntrinsicInst>(CB), LVI);
-    return true;
+    return processAbsIntrinsic(&cast<IntrinsicInst>(CB), LVI);
   }
 
   if (auto *MM = dyn_cast<MinMaxIntrinsic>(&CB)) {
-    processMinMaxIntrinsic(MM, LVI);
-    return true;
+    return processMinMaxIntrinsic(MM, LVI);
   }
 
   if (auto *WO = dyn_cast<WithOverflowInst>(&CB)) {
     if (WO->getLHS()->getType()->isIntegerTy() && willNotOverflow(WO, LVI)) {
-      processOverflowIntrinsic(WO, LVI);
-      return true;
+      return processOverflowIntrinsic(WO, LVI);
     }
   }
 
   if (auto *SI = dyn_cast<SaturatingInst>(&CB)) {
     if (SI->getType()->isIntegerTy() && willNotOverflow(SI, LVI)) {
-      processSaturatingInst(SI, LVI);
-      return true;
+      return processSaturatingInst(SI, LVI);
     }
   }
 
   bool Changed = false;
 
   // Deopt bundle operands are intended to capture state with minimal
   // perturbance of the code otherwise.  If we can find a constant value for
   // any such operand and remove a use of the original value, that's