[InlineCost]: fold CmpInstr when next iteration of a recursive call can simplify the Cmp decision.

Author: hassnaaHamdi

llvm/include/llvm/Analysis/InlineCost.h

@@ -236,7 +236,7 @@ struct InlineParams {
   std::optional<bool> EnableDeferral;
 
   /// Indicate whether we allow inlining for recursive call.
-  std::optional<bool> AllowRecursiveCall = true;
+  std::optional<bool> AllowRecursiveCall = false;
 };
 
 std::optional<int> getStringFnAttrAsInt(CallBase &CB, StringRef AttrKind);

llvm/lib/Analysis/InlineCost.cpp

@@ -445,6 +445,7 @@ class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
   bool canFoldInboundsGEP(GetElementPtrInst &I);
   bool accumulateGEPOffset(GEPOperator &GEP, APInt &Offset);
   bool simplifyCallSite(Function *F, CallBase &Call);
+  bool simplifyCmpInst(Function *F, CmpInst &Cmp);
   bool simplifyInstruction(Instruction &I);
   bool simplifyIntrinsicCallIsConstant(CallBase &CB);
   bool simplifyIntrinsicCallObjectSize(CallBase &CB);
@@ -1171,10 +1172,6 @@ class InlineCostCallAnalyzer final : public CallAnalyzer {
   std::optional<CostBenefitPair> getCostBenefitPair() { return CostBenefit; }
   bool wasDecidedByCostBenefit() const { return DecidedByCostBenefit; }
   bool wasDecidedByCostThreshold() const { return DecidedByCostThreshold; }
-  bool shouldCheckRecursiveCall() {
-    return IsRecursiveCall && AllowRecursiveCall;
-  }
-  bool shouldInlineRecursiveCall(CallBase &Call);
 };
 
 // Return true if CB is the sole call to local function Callee.
@@ -1681,6 +1678,68 @@ bool CallAnalyzer::visitGetElementPtr(GetElementPtrInst &I) {
   return isGEPFree(I);
 }
 
+/// Simplify \p Cmp if RHS is const and we can ValueTrack LHS,
+// This handles the case when the Cmp instruction is guarded a recursive call
+// that will cause the Cmp to fail/succeed for the next iteration.
+bool CallAnalyzer::simplifyCmpInst(Function *F, CmpInst &Cmp) {
+  // Bail out if the RHS is NOT const:
+  if (!isa<Constant>(Cmp.getOperand(1)))
+    return false;
+  auto *CmpOp = Cmp.getOperand(0);
+  // Iterate over the users of the function to check if it's a recursive function:
+  for (auto *U : F->users()) {
+    CallInst *Call = dyn_cast<CallInst>(U);
+    if (!Call || Call->getFunction() != F)
+      continue;
+    auto *CallBB = Call->getParent();
+    auto *Predecessor = CallBB->getSinglePredecessor();
+    // Only handle the case when the callsite has a single predecessor:
+    if (!Predecessor)
+      continue;
+
+    auto *Br = dyn_cast<BranchInst>(Predecessor->getTerminator());
+    if (!Br || Br->isUnconditional())
+      continue;
+    // Check if the Br condition is the same Cmp instr we are investigating:
+    auto *CmpInstr = dyn_cast<CmpInst>(Br->getCondition());
+    if (!CmpInstr || CmpInstr != &Cmp)
+      continue;
+    // Check if there are any arg of the recursive callsite is affecting the cmp instr:
+    bool ArgFound = false;
+    Value *FuncArg = nullptr, *CallArg = nullptr;
+    for (unsigned ArgNum = 0; ArgNum < F->arg_size() && ArgNum < Call->arg_size(); ArgNum ++) {
+      FuncArg = F->getArg(ArgNum);
+      CallArg = Call->getArgOperand(ArgNum);
+      if ((FuncArg == CmpOp) &&
+          (CallArg != CmpOp)) {
+        ArgFound = true;
+        break;
+      }
+    }
+    if (!ArgFound)
+      continue;
+    // Now we have a recursive call that is guarded by a cmp instruction.
+    // Check if this cmp can be simplified:
+    SimplifyQuery SQ(DL, dyn_cast<Instruction>(CallArg));
+    DomConditionCache DC;
+    DC.registerBranch(Br);
+    SQ.DC = &DC;
+    DominatorTree DT(*F);
+    SQ.DT = &DT;
+    Value *simplifiedInstruction = llvm::simplifyInstructionWithOperands(CmpInstr, {CallArg, Cmp.getOperand(1)}, SQ);
+    if (!simplifiedInstruction)
+      continue;
+    if (auto *ConstVal = dyn_cast<llvm::ConstantInt>(simplifiedInstruction)) {
+      bool isTrueSuccessor = CallBB == Br->getSuccessor(0);
+      SimplifiedValues[&Cmp] = ConstVal;
+      if (ConstVal->isOne())
+        return !isTrueSuccessor;
+      return isTrueSuccessor;
+    }
+  }
+  return false;
+}
+
 /// Simplify \p I if its operands are constants and update SimplifiedValues.
 bool CallAnalyzer::simplifyInstruction(Instruction &I) {
   SmallVector<Constant *> COps;
@@ -2065,6 +2124,10 @@ bool CallAnalyzer::visitCmpInst(CmpInst &I) {
   if (simplifyInstruction(I))
     return true;
 
+  // Try to handle comparison that can be simplified using ValueTracking.
+  if (simplifyCmpInst(I.getFunction(), I))
+    return true;
+
   if (I.getOpcode() == Instruction::FCmp)
     return false;
 
@@ -2900,68 +2963,6 @@ InlineResult CallAnalyzer::analyze() {
   return finalizeAnalysis();
 }
 
-bool InlineCostCallAnalyzer::shouldInlineRecursiveCall(CallBase &Call) {
-  CallInst *CI = cast<CallInst>(&Call);
-  auto CIB = CI->getParent();
-  // Only handle case when we have sinlge predecessor
-  if (auto Predecessor = CIB->getSinglePredecessor()) {
-    BranchInst *Br = dyn_cast<BranchInst>(Predecessor->getTerminator());
-    if (!Br || Br->isUnconditional()) {
-      return false;
-    }
-    Value *Var = Br->getCondition();
-    CmpInst *CmpInstr = dyn_cast<CmpInst>(Var);
-    if (CmpInstr && !isa<Constant>(CmpInstr->getOperand(1))) {
-      // Current logic of ValueTracking/DomConditionCache works only if RHS is
-      // constant.
-      return false;
-    }
-    unsigned ArgNum = 0;
-    Value *FuncArg = nullptr, *CallArg = nullptr;
-    // Check which func argument the cmp instr is using:
-    for (; ArgNum < CI->getFunction()->arg_size(); ArgNum++) {
-      FuncArg = CI->getFunction()->getArg(ArgNum);
-      CallArg = CI->getArgOperand(ArgNum);
-      if (CmpInstr) {
-        if ((FuncArg == CmpInstr->getOperand(0)) &&
-            (CallArg != CmpInstr->getOperand(0)))
-          break;
-      } else if (FuncArg == Var && (CallArg != Var))
-        break;
-    }
-    // Only handle the case when a func argument controls the cmp instruction:
-    if (ArgNum < CI->getFunction()->arg_size()) {
-      bool isTrueSuccessor = CIB == Br->getSuccessor(0);
-      if (CmpInstr) {
-        SimplifyQuery SQ(CI->getFunction()->getDataLayout(),
-                         dyn_cast<Instruction>(CallArg));
-        DomConditionCache DC;
-        DC.registerBranch(Br);
-        SQ.DC = &DC;
-        DominatorTree DT(*CI->getFunction());
-        SQ.DT = &DT;
-        Value *simplifiedInstruction = llvm::simplifyInstructionWithOperands(
-            CmpInstr, {CallArg, CmpInstr->getOperand(1)}, SQ);
-        if (!simplifiedInstruction)
-          return false;
-        if (auto *ConstVal =
-                dyn_cast<llvm::ConstantInt>(simplifiedInstruction)) {
-          if (ConstVal->isOne())
-            return !isTrueSuccessor;
-          return isTrueSuccessor;
-        }
-      } else {
-        if (auto *ConstVal = dyn_cast<llvm::ConstantInt>(CallArg)) {
-          if (ConstVal->isOne())
-            return !isTrueSuccessor;
-          return isTrueSuccessor;
-        }
-      }
-    }
-  }
-  return false;
-}
-
 void InlineCostCallAnalyzer::print(raw_ostream &OS) {
 #define DEBUG_PRINT_STAT(x) OS << "      " #x ": " << x << "\n"
   if (PrintInstructionComments)
@@ -3193,12 +3194,6 @@ InlineCost llvm::getInlineCost(
 
   LLVM_DEBUG(CA.dump());
 
-  // Check if callee function is recursive:
-  if (ShouldInline.isSuccess()) {
-    if (CA.shouldCheckRecursiveCall() && !CA.shouldInlineRecursiveCall(Call))
-      return InlineCost::getNever("deep recursion");
-  }
-
   // Always make cost benefit based decision explicit.
   // We use always/never here since threshold is not meaningful,
   // as it's not what drives cost-benefit analysis.
@@ -3241,13 +3236,6 @@ InlineResult llvm::isInlineViable(Function &F) {
 
       // Disallow recursive calls.
       Function *Callee = Call->getCalledFunction();
-      // This function is called when we have "alwaysinline" attribute.
-      // If we allowed the inlining here given that the recursive inlining is
-      // allowed, then there will be problem in the second trial of inlining,
-      // because the Inliner pass allow only one time inlining and then it
-      // inserts "noinline" attribute which will be in conflict with the
-      // attribute of "alwaysinline" so, "alwaysinline" for recursive function
-      // will be disallowed to avoid conflict of attributes.
       if (&F == Callee)
         return InlineResult::failure("recursive call");
 

llvm/test/Transforms/Inline/inline-remark.ll

@@ -56,6 +56,6 @@ define void @test3() {
 }
 
 ; CHECK: attributes [[ATTR1]] = { "inline-remark"="(cost=25, threshold=0)" }
-; CHECK: attributes [[ATTR2]] = { "inline-remark"="(cost=0, threshold=0)" }
+; CHECK: attributes [[ATTR2]] = { "inline-remark"="(cost=never): recursive" }
 ; CHECK: attributes [[ATTR3]] = { "inline-remark"="unsupported operand bundle; (cost={{.*}}, threshold={{.*}})" }
 ; CHECK: attributes [[ATTR4]] = { alwaysinline "inline-remark"="(cost=never): recursive call" }