[PATCH] D16381: Infrastructure to allow use of PGO in inliner

[David Li via llvm-commits]

davidxl added a comment.

We need to add more test cases. For instance

1. two callsites to one callee all get inlined -- the callee should have zero count left
2. scaling testing -- especially interesting in the context of top-down inlining (in the future we may have more heuristics to enable more top down inining, there are existing heuristics that can be used to trigger the case):

a ---> (200) ---> c
b ---> (300) ---> c
where c has entry count of 500

c-->(200) --> e
d -->(300) --> e
where e has entry count of 500.

Suppose there are two inlines in the following order:  a->c, and a->e

After inlining, testing e's entry count is properly updated.


================
Comment at: include/llvm/Analysis/InlineCost.h:44
@@ +43,3 @@
+/// This class mimics block frequency analysis on CGSCC level. Block frequency
+/// info is computed on demand and cached unless they are invalidated.
+class BlockFrequencyAnalysis {
----------------
suggested comment change: ... on demand, cached (for callees), and incrementally updated (for callers). The caller BFI is invalidated after inlining is done for (the caller).

================
Comment at: include/llvm/Analysis/InlineCost.h:53
@@ +52,3 @@
+  BlockFrequencyInfo *getBlockFrequencyInfo(Function *);
+  /// \brief Invalidates block freqquency info for a function.
+  void invalidateBlockFrequencyInfo(Function *);
----------------
Typo -- frequency

================
Comment at: lib/Analysis/InlineCost.cpp:621
@@ +620,3 @@
+  BasicBlock *CallBB = Call->getParent();
+  if (HasPGOCounts) {
+    Optional<uint64_t> CallSiteCount = llvm::getBlockCount(CallBB, BFA);
----------------
This guard is wrong :1) it should check caller entry count, not callee entry count. 2) hasPGOCounts also depends on other conditions.

In fact, this condition can be removed.

================
Comment at: lib/Analysis/InlineCost.cpp:1557
@@ +1556,3 @@
+
+Optional<uint64_t> llvm::getBlockCount(BasicBlock *BB,
+                                       BlockFrequencyAnalysis *BFA) {
----------------
Add a comment for the function.

================
Comment at: lib/Analysis/InlineCost.cpp:1570
@@ +1569,3 @@
+
+////// Block frequency analysis for functions in the SCC
+BlockFrequencyAnalysis::~BlockFrequencyAnalysis() {
----------------
Irrelevant comment line?


http://reviews.llvm.org/D16381