[PATCH] D92780: [InlineCost] Implement cost-benefit-based inliner

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:481
+  // The cumulative cost at the beginning of the basic block being analyzed.  At
+  // the end of analyzing each basic block, "CostAtBBStart - Cost" represents
+  // the size of that basic block.
----------------
In comment, it should be Cost - CostAtBBStart


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:632
+      // we define a cold basic block to be one that's never executed.
+      if (BlockFrequencyInfo *BFI = GetBFI ? &(GetBFI(F)) : nullptr) {
+        auto ProfileCount = BFI->getBlockProfileCount(BB);
----------------
Are these guards needed? It is already checked in costBenefitAnalysis.  Perhaps move the checking in a common place and set it to boolean variable: costBenefitAnalysisEnabled and the value of the variable can be checked here.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:634
+        auto ProfileCount = BFI->getBlockProfileCount(BB);
+        if (ProfileCount.hasValue() && ProfileCount.getValue() == 0)
+          ColdSize += Cost - CostAtBBStart;
----------------
should assert that it has value 


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:692
+    auto CallerProfileCount = CallerBFI->getBlockProfileCount(CallerBB);
+    if (!CallerProfileCount.hasValue())
+      return None;
----------------
checker CallerEntry count earlier than this check can be removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92780/new/

https://reviews.llvm.org/D92780