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

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 2 inline comments as done.
vsk added a comment.

Thanks for the review.



================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:184
+      MaxAncestor->getSinglePredecessor() == MaxAncestor)
+    return {};
+
----------------
sebpop wrote:
> 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.
Good point, I'll add a comment.


================
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) {
----------------
sebpop wrote:
> 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...
Good point. Although.. marking outlined calls as noreturn when appropriate and outlining more than once per function could achieve most of the win.


================
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)
----------------
sebpop wrote:
> 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.
I have tried it out internally and saw a substantial increase in outlining, but haven't looked into performance impact yet. I'll send a separate patch when I have more data.


https://reviews.llvm.org/D53627