[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:429
+          if (TracePartialInlining.Full)
+            dbgs() << "Found exit block = " << (*SI)->getName() << "\n";
+          if (ExitBlock)
----------------
Consider making this into a message with -Rpass-missed=partial-inining


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:497
+        continue;
+      if (TracePartialInlining.Full)
+        dbgs() << "Dominates more than one block. Single predecessor.\n";
----------------
This trace does not look useful. Consider removing it.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:508
+      if (TracePartialInlining.Full) {
+        dbgs() << "Single Exit Block = " << ExitBlock->getName() << "\n";
+        dbgs() << "OutlineRegionCost = " << OutlineRegionCost << "\n";
----------------
merge these into one output stream


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:514
+      if (OutlineRegionCost < MinOutlineRegionCost) {
+        if (TracePartialInlining.Full) {
+          dbgs() << "Inline cost-savings smaller than "
----------------
Consider changing this into -Rpass-analysis=partial-inlining


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1151
+    } else
+      if (TracePartialInlining.Sparse)
+        dbgs() << "CodeExtractor did not extract code region\n";
----------------
-Rpass-missed


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1228
   if (F->hasAddressTaken())
-    return nullptr;
+    return {false,nullptr};
 
----------------
clang-format and similar other places.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1243
 
-  std::unique_ptr<FunctionOutliningInfo> OI = computeOutliningInfo(F);
+  if (TracePartialInlining.Full) {
+    dbgs() << ">>>>>> Original Function >>>>>>\n";
----------------
Consider removing this trace. Should be handled by -print-before-xxx and -print-after-xxx


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:1464
   }
+  if (TracePartialInlining.Full && Changed)
+    dbgs() << "Partially inlined at least one function.\n";
----------------
You can achieve the same thing with -Rpass=partial-inlining. Consider remove this tracing output.


================
Comment at: test/Transforms/CodeExtractor/PartialInlinePGORegion.ll:5
+; of region containing more than one BB.
+define signext i32 @bar(i32 signext %value, i32 signext %ub) #0 !prof !30 {
+entry:
----------------
Perhaps add another case with multiple regions outlined?


Repository:
  rL LLVM

https://reviews.llvm.org/D38190