[PATCH] D53887: [HotColdSplitting] [WIP] Outline more than once per function

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:348
+      // function must be cold.
+      if (pred_empty(&PredBB)) {
+        ColdRegion.EntireFunctionCold = true;
----------------
hiraditya wrote:
> vsk wrote:
> > hiraditya wrote:
> > > I think, this can go before the previous check.
> > I think that would cause splitting to fail when a predecessor outside of the cold region is the entry block. Am I missing something?
> When SinkPostDom is true and PredBB is the entry, we can just return.
I see what you mean.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:480
+
+    LLVM_DEBUG({
+      dbgs() << "Found cold block:\n";
----------------
hiraditya wrote:
> Creating scope '{}' may not be required.
It's not, I just find it easier to read multiple statements when there are surrounding curly braces.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:517
+      if (!isProfitableToOutline(SubRegion, TTI)) {
+        LLVM_DEBUG(dbgs() << "Skipping outlining; not profitable to outline\n");
+        continue;
----------------
sebpop wrote:
> hiraditya wrote:
> > Maybe print the region which didn't get outlined, or at least the root-block.
> I think that would generate too much debug output.  Maybe completely remove the LLVM_DEBUG stmt.
I think some form of debug statement here would help to tune the outlining threshold. Could we keep it for now? I'll remove some of the other redundant debug statements to keep the output reasonable.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:522
+      Function *Outlined =
+          extractColdRegion(SubRegion, DT, BFI, TTI, ORE, /*Count=*/1);
+      if (Outlined) {
----------------
tejohnson wrote:
> The last argument should be changed from a constant 1 to a running count of outlined regions. Otherwise all outlined functions will have the same name.
Thanks for pointing this out.


https://reviews.llvm.org/D53887