[PATCH] D92780: [InlineCost] Implement cost-benefit-based inliner
David Li via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 8 09:08:05 PST 2020
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
More information about the llvm-commits
mailing list