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

[Kazu Hirata via Phabricator via llvm-commits]

kazu marked 10 inline comments as done.
kazu added a comment.

PTAL.  Thanks!



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


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:483
+  // The cost of cold basic blocks.
+  int ColdSize = 0;
+
----------------
davidxl wrote:
> 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?
I'm computing the static size here.  CostAtBBStart is just a copy of Cost at the beginning of the basic block being processed so that we can compute the size of the the basic block in case it's cold.

Since I'm just copying Cost, I'd like to use the same type as Cost.


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

I've moved up the entry count for Callee.

For the caller, do I need to check its entry count?  All I need is the profile count at the call site.  In any event, I've moved up the code to obtain the profile count at the call site.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:725
+      assert(ProfileCount.hasValue());
+      WeightedSavings += ProfileCount.getValue() * CurrentSavings;
+    }
----------------
mtrofin wrote:
> for really large profile counts (e.g. worker thread that blocks, waiting for work) this may overflow. Do we care?
I've switched to 128-bit APInt.  I've put justification for the bit size as a comment for CycleSavings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92780