[PATCH] D38190: Partial Inlining with multi-region outlining based on PGO information

[Graham Yiu via Phabricator via llvm-commits]

gyiu marked 5 inline comments as done.
gyiu added inline comments.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:87
+    "cold-branch-ratio", cl::init(0.1), cl::Hidden,
+    cl::desc("Minimum BranchProbability to consider a region cold."));
 
----------------
davidxl wrote:
> gyiu wrote:
> > davidxl wrote:
> > > Why using BP to determine region coldness instead of PSI and profile count?
> > No particular reason, mainly for convenience.  I figure I would still need to compare the profile count of a block to its sibling blocks to know its relative coldness.  Is PSI and profile count more accurate?
> Using PSI gives you information about global hotness of a block, so that the compiler only focus on regions that are globally hot or cold. 
Interesting.  One concern I have, though, is whether we should check the predecessor block for 'isHotBB', or the cold successor block for 'isColdBB', or both.  Checking if the predecessor is globally hot will still require some branch probability ratio to indicate a relatively cold successor block.  Similarly with a globally cold successor, we will still need to check the predecessor block for relative hotness.  Checking both could be good, though we will likely miss a lot of opportunities.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:428
+  // Return if we don't have profiling information.
+  if (!PSI->hasProfileSummary() || !PSI->hasInstrumentationProfile())
+    return std::unique_ptr<FunctionOutliningMultiRegionInfo>();
----------------
davidxl wrote:
> This skips SamplePGO. Is it intended? If yes, only the second check should be enough.
Yeah, I'm not confident that the sampling profile will give us proper coldness information, and may lead us to outline more than we should.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:437
+    return BlockList.size() > 1 &&
+           std::distance(pred_begin(Dom), pred_end(Dom)) == 1;
+  };
----------------
davidxl wrote:
> Is this too restrictive? Single entry region  does not mean the entry block can not have multiple predecessors.
I guess I'm unsure what a dominator block with multiple predecessors actually means in this case, because the code looks for a cold successor block to a branch.  If that cold successor has multiple predecessors, then doesn't that we can enter this region through multiple blocks, hence not be single entry at all?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:506
+      if (SuccProb > MinBranchProbability)
+        continue;
+      if (TracePartialInliningFull) {
----------------
davidxl wrote:
> should check if the the Succ block is actually cold enough.
You mean check the global coldness of the Succ Block?  I guess this is where we need to weigh the pros and cons of global coldnress vs. relative coldness (similarly for hotness).  Are you thinking we should check relative AND global coldness?  Would this be reasonable in a situation where the callgraph is disjoint (eg. building a shared library) and we can't make reasonable comparisons of the counters between subgraphs?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:545
+        // can't outline if region contains a return block
+        if (IsReturnBlock(ExitBlock))
+          continue;
----------------
davidxl wrote:
> Why this limitation? This should be handled by the code extractor
You're right.  I didn't realize that.


Repository:
  rL LLVM

https://reviews.llvm.org/D38190