[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:81
+                            cl::init(8), cl::ZeroOrMore,
+                            cl::desc("Inline savings multiplier"));
+
----------------
Make the documentation string more detailed.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:480
+  // The cumulative cost at the beginning of the basic block being analyzed.
+  int CostAtBBStart = 0;
+
----------------
To differentiate existing (static) cost which models size, perhaps name it 'weightedCostAtBBStart'? or 'DynamicCostAtBBStart'?


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:483
+  // The cost of cold basic blocks.
+  int ColdSize = 0;
+
----------------
mtrofin wrote:
> can size be negative? Should it be int64_t?
> 
> Also, is it a size, or it's a cost - judging from its usage, probably the latter. Should it be ColdSizeCost?
The naming needs to be consistent.  Is it computing static size or weighted cost?


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:626
+    // Keep track of the cost of cold basic blocks.  For now, we define a cold
+    // basic block to be one that's never executed.
+    if (BlockFrequencyInfo *BFI = GetBFI ? &(GetBFI(F)) : nullptr) {
----------------
If function entry has no count, early skip -- to avoid compile time impact to nonPGO build.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:670
+
+    BlockFrequencyInfo *BFI = GetBFI ? &(GetBFI(F)) : nullptr;
+    if (!BFI)
----------------
Check function entry count first for both caller and callee.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:670
+
+    BlockFrequencyInfo *BFI = GetBFI ? &(GetBFI(F)) : nullptr;
+    if (!BFI)
----------------
davidxl wrote:
> Check function entry count first for both caller and callee.
Is 'F' callee? if so, name BFI CalleeBFI .


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:681
+    auto CallerProfileCount = CallerBFI->getBlockProfileCount(CallerBB);
+    if (!CallerProfileCount.hasValue())
+      return None;
----------------
if caller entry has count, this should assert to be true.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:684
+
+    for (auto &BB : F) {
+      auto ProfileCount = BFI->getBlockProfileCount(&BB);
----------------
No need for this loop if the entry count is checked at beginning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92780