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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:160
+/// Mark \p F cold. Return true iff it's changed.
+static bool markEntireFunctionCold(Function &F) {
+  assert(!F.hasFnAttribute(Attribute::OptimizeNone) && "Can't mark this cold");
----------------
This function set MinSize, so I think the function name should be something like markMinSize().


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:410
+
+      ColdRegion.Blocks.emplace_back(&SuccBB, SuccScore);
+      ++SuccIt;
----------------
We should have additional check before adding SuccBB into Blocks (maybe successors of SuccBB). If a dom-frontier have a phi taking different incoming values from multiple cold blocks, it will assert. In the example below, %if.end takes different incoming values from %coldbb and %coldbb2. 

```
define void @foo(i32 %cond) {
entry:
  %tobool = icmp eq i32 %cond, 0
  br i1 %tobool, label %if.end, label %coldbb

coldbb:
  call void (...) @sink()
  br i1 undef, label %if.end, label %coldbb2

coldbb2:
  br label %if.end

if.end:
  %p = phi i32 [0, %entry], [1, %coldbb], [3, %coldbb2]
  ret void
}

declare void @sink(...) cold
```



================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:511
+      LLVM_DEBUG(dbgs() << "Entire function is cold\n");
+      return markEntireFunctionCold(F);
+    }
----------------
I don't think it's good idea to add MinSize in this case. It is possible that a hot function or main() itself can be successfully ended up with exit(0). 


https://reviews.llvm.org/D53887