Fix FunctionPropertiesAnalysis updating callsite in 1-BB loop

[mtrofin]

Summary

If the callsite is in a single BB loop, we need to exclude the BB from
the successor set (in which it'd be a member), because that set forms a
boundary at which we stop traversing the CFG, when re-ingesting BBs
after inlining; but after inlining, the callsite BB's new successors
should be visited.

[kazu]

Thank you for the fix. LGTM modulo some rewording of the comment.

llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp

138–142

I understand the motivation and the fix, but I am wondering if we can make the comment a little easier to understand.

// Exclude the CallSiteBB.  finish() traverses the resulting CFG from CallSiteBB                                                                  
// up to and including Successors.  Including the CallSiteBB in Successors would                                                                  
// prematurely stop the traversal when it happens to be its own successor.

Nit: "would prematurely stop the traversal" sounds more natural than "would stop prematurely the traversal".

llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp

FunctionPropertiesUpdater::FunctionPropertiesUpdater(
 
   // The caller's entry BB may change due to new alloca instructions.
   LikelyToChangeBBs.insert(&*Caller.begin());
 
   // The successors may become unreachable in the case of `invoke` inlining.
   // We track successors separately, too, because they form a boundary, together
   // with the CB BB ('Entry') between which the inlined callee will be pasted.
   Successors.insert(succ_begin(&CallSiteBB), succ_end(&CallSiteBB));
+
+  // Exclude the CallSiteBB, if it happens to be its own successor (1-BB loop).
+  // We are only interested in BBs the graph moves past the callsite BB to
+  // define the frontier past which we don't want to re-process BBs. Including
+  // the callsite BB in this case would prematurely stop the traversal in
+  // finish().
+  Successors.erase(&CallSiteBB);
+
   for (const auto *BB : Successors)
     LikelyToChangeBBs.insert(BB);
 
   // Commit the change. While some of the BBs accounted for above may play dual
   // role - e.g. caller's entry BB may be the same as the callsite BB - set
   // insertion semantics make sure we account them once. This needs to be
   // followed in `finish`, too.
   for (const auto *BB : LikelyToChangeBBs)

llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp

)IR");
   FPU.finish(LINew);
   EXPECT_EQ(static_cast<size_t>(FPI.BasicBlockCount),
             F1->getBasicBlockList().size() - 1);
   EXPECT_EQ(static_cast<size_t>(FPI.TotalInstructionCount),
             F1->getInstructionCount() - 2);
   EXPECT_EQ(FPI, FunctionPropertiesInfo::getFunctionPropertiesInfo(*F1, LINew));
 }
 
+TEST_F(FunctionPropertiesAnalysisTest, InlineSameLoopBB) {
+  LLVMContext C;
+  std::unique_ptr<Module> M = makeLLVMModule(C,
+                                             R"IR(
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+declare i32 @a()
+declare i32 @b()
+
+define i32 @f1(i32 %a) {
+entry:
+  br label %loop
+loop:
+  %i = call i32 @f2(i32 %a)
+  %c = icmp slt i32 %i, %a
+  br i1 %c, label %loop, label %end
+end:
+  %r = phi i32 [%i, %loop], [%a, %entry]
+  ret i32 %r
+}
+
+define i32 @f2(i32 %a) {
+  %cnd = icmp slt i32 %a, 0
+  br i1 %cnd, label %then, label %else
+then:
+  %r1 = call i32 @a()
+  br label %end
+else:
+  %r2 = call i32 @b()
+  br label %end
+end:
+  %r = phi i32 [%r1, %then], [%r2, %else]
+  ret i32 %r
+}
+)IR");
+
+  Function *F1 = M->getFunction("f1");
+  CallBase *CB = findCall(*F1);
+  EXPECT_NE(CB, nullptr);
+
+  FunctionPropertiesInfo ExpectedInitial;
+  ExpectedInitial.BasicBlockCount = 3;
+  ExpectedInitial.TotalInstructionCount = 6;
+  ExpectedInitial.BlocksReachedFromConditionalInstruction = 2;
+  ExpectedInitial.Uses = 1;
+  ExpectedInitial.DirectCallsToDefinedFunctions = 1;
+  ExpectedInitial.MaxLoopDepth = 1;
+  ExpectedInitial.TopLevelLoopCount = 1;
+
+  FunctionPropertiesInfo ExpectedFinal = ExpectedInitial;
+  ExpectedFinal.BasicBlockCount = 6;
+  ExpectedFinal.DirectCallsToDefinedFunctions = 0;
+  ExpectedFinal.BlocksReachedFromConditionalInstruction = 4;
+  ExpectedFinal.TotalInstructionCount = 12;
+
+  auto FPI = buildFPI(*F1);
+  EXPECT_EQ(FPI, ExpectedInitial);
+
+  FunctionPropertiesUpdater FPU(FPI, *CB);
+  InlineFunctionInfo IFI;
+  auto IR = llvm::InlineFunction(*CB, IFI);
+  EXPECT_TRUE(IR.isSuccess());
+  FPU.finish(*LI);
+  EXPECT_EQ(FPI, ExpectedFinal);
+}
+
 } // end anonymous namespace