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

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

please also add test cases.



================
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."));
 
----------------
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. 


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:165
 
+static bool TracePartialInliningSparse = false;
+static bool TracePartialInliningFull = false;
----------------
Make these two member variables.


================
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>();
----------------
This skips SamplePGO. Is it intended? If yes, only the second check should be enough.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:437
+    return BlockList.size() > 1 &&
+           std::distance(pred_begin(Dom), pred_end(Dom)) == 1;
+  };
----------------
Is this too restrictive? Single entry region  does not mean the entry block can not have multiple predecessors.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:506
+      if (SuccProb > MinBranchProbability)
+        continue;
+      if (TracePartialInliningFull) {
----------------
should check if the the Succ block is actually cold enough.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:520
+      // We can only outline single exit regions (for now).
+      if (auto *ExitBlock = IsSingleExit(DominateVector)) {
+        int OutlineRegionCost = 0;
----------------
if (!IsSingleExit(..))) 
   continue;


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


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1437
+  bool AnyInlined = false;
+  for (User *User : Users) {
+    CallSite CS = getCallSite(User);
----------------
Most of the code here can be shared with the existing partial inliner. Refactor them to avoid code duplication?


Repository:
  rL LLVM

https://reviews.llvm.org/D38190