[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: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");
----------------
junbuml wrote:
> This function set MinSize, so I think the function name should be something like markMinSize().
That's true, but as a follow-up, I'd like to have this helper add Attribute::Cold, and move the function to the end of the text segment. Would it be all right to keep the aspirational name?


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:282
     // that it's cold.
-    assert(!OutF->hasFnAttribute(Attribute::OptimizeNone) &&
-           "An outlined function should never be marked optnone");
-    OutF->addFnAttr(Attribute::MinSize);
+    markEntireFunctionCold(*OutF);
 
----------------
junbuml wrote:
> Considering the case below, I believed we should not mark the MinSize here as well. 
> 
> 
> ```
> void foo(bool c) {
>   if(c) {
>     ....
>     return;
>   }
> 
>    // hot code
>    exit (0);
> }
> ```
I think this is illustrative of a more general problem: I don't think we should treat NoReturn functions as cold at all. On Darwin, functions like longjmp are NoReturn but warm. Our kernel uses continuation functions (conceptually similar to longjmp) to reduce stack usage: these are also NoReturn but hot.

IMO, noreturn functions which are actually cold should be marked as such (abort, etc., but not exit). That's what we do on Darwin.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:410
+
+      ColdRegion.Blocks.emplace_back(&SuccBB, SuccScore);
+      ++SuccIt;
----------------
junbuml wrote:
> 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
> ```
> 
As noted in llvm.org/PR39545, I'll send out a separate review to address this.


https://reviews.llvm.org/D53887