[PATCH] D53627: [HotColdSplitting] Identify larger cold regions using domtree queries

[Sebastian Pop via Phabricator via llvm-commits]

sebpop accepted this revision.
sebpop added a comment.
This revision is now accepted and ready to land.

ok



================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:184
+      MaxAncestor->getSinglePredecessor() == MaxAncestor)
+    return {};
+
----------------
When the MaxAncestor has no predecessors we may be able to outline the second largest cold region that has a predecessor.  Maybe add a comment about this.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:186
+
+  // Filter out predecessors not dominated by the max ancestor.
+  auto EraseIt = remove_if(ColdRegion, [&](BasicBlock *PredBB) {
----------------
Maybe add a FIXME note: the blocks not dominated by the max ancestor may be extracted as other smaller cold regions.  Not sure how much benefit that would give...


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:221
+  // TODO: Consider outlining regions with just 1 block, but more than some
+  // threshold of instructions.
+  if (ColdRegion.size() == 1)
----------------
Agreed. I think the threshold can be pretty low given that the code is cold.
You could even try to remove the condition (ColdRegion.size() == 1) and see what happens: code size and perf.


https://reviews.llvm.org/D53627