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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:45
           "Number of callsites functions partially inlined into.");
+STATISTIC(NewNumPartialInlined,
+           "Number of callsites functions (NEW) partially inlined into.");
----------------
Make the name more meaningful? New does not tells us anything.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:63
+// Command line option to debug partial-inlining. The default is false:
+static cl::opt<bool>
+    TracePartialInlining("trace-partial-inlining", cl::init(false),
----------------
Make this an enum option?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:66
+                           cl::Hidden, cl::desc("Trace partial inlining"));
+static cl::opt<bool>
+    TracePartialInliningSparse("trace-partial-inlining-sparse", cl::init(false),
----------------
Merge with the enum trace option


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:77
+    "min-region-size-ratio", cl::init(0.1), cl::Hidden,
+    cl::desc("Minimum ratio comparing relative sizes of each "
+             "outline candidate and original function"));
----------------
Indicate the purpose of the ratio, e.g, 'to decide xxx'


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:83
+                             cl::desc("Minimum block executions to consider "
+                                      "its BranchProbabilityInfo valid"));
+// Use for tuning outlining heuristics.
----------------
Why is option needed?


================
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."));
 
----------------
Why using BP to determine region coldness instead of PSI and profile count?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:161
   bool run(Module &M);
-  Function *unswitchFunction(Function *F);
+  std::pair<bool, Function *> unswitchFunction(Function *F);
 
----------------
Add documentation.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:417
+  VisitedMap[CurrEntry] = true;
+  // Use Depth First Search on the basic blocks to find CFG edges that are
+  // considered cold.
----------------
Add a high level description of how the cost/savings are computed below.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1060
+
+    Function *OutlinedFunc = CodeExtractor(RegionInfo.Region, &DT, /*AggregateArgs*/ false,
+                                 ClonedFuncBFI.get(), &BPI)
----------------
Is DT updated by the extractor?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1150
   if (F->hasAddressTaken())
-    return nullptr;
+    return std::make_pair(false,nullptr);
 
----------------
return { false, nullptr };


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1173
+  // implies we have profiling information.
+  if (PSI->hasProfileSummary() && !DisableMultiRegionPartialInline) {
+    std::unique_ptr<FunctionOutliningMultiRegionInfo> OMRI =
----------------
Better check if the function has EntryCount


Repository:
  rL LLVM

https://reviews.llvm.org/D38190