[hot-cold-split] Name split functions with ".cold" suffix

[tejohnson]

Summary

The current default of appending "_"+entry block label to the new
extracted cold function breaks demangling. Change this to ".cold" for hot
cold splitting (other uses of CodeExtractor still use old naming), since
the demangler knows how to handle suffixes starting with ".". This
is also consistent with how gcc names the split cold functions.

[sebpop]

lib/Transforms/Utils/CodeExtractor.cpp

678

In the case of more than one region extracted from a function, this code may create functions with identical names which would create link problems.

[tejohnson]

lib/Transforms/Utils/CodeExtractor.cpp

678

Will the hot cold split pass extract more than one cold function from a single function?

[sebpop]

lib/Transforms/Utils/CodeExtractor.cpp

678

Yes with https://reviews.llvm.org/D53588

[tejohnson]

lib/Transforms/Utils/CodeExtractor.cpp

678

Ok I see. It would make sense then to append a unique id, which could be the header name. Also we may in general want to change the "_" here to a "." so that the demangler handles it appropriately, regardless of whether we change the suffix after that deliminator.

I did see in my internal project that a lot of the cold split funcs actually ended with "_" without any header label - I need to go back and investigate - do we have empty header names in some cases? Because if so, with splitting into multiple funcs we will also end up with name collisions. Let me take another look...

[tejohnson]

lib/Transforms/Utils/CodeExtractor.cpp

678

When the IR is generated by a release built clang, the bb labels are empty. So that will cause issues with your D53588 patch, since all outlined funcs will be named original name + "_". So they need to use some other id when the name is empty. Perhaps the client of CodeExtractor needs to supply a unique id (for hot cold split you could just enumerate the split out functions for each original function and use that as a unique id).

[tejohnson]

lib/Transforms/Utils/CodeExtractor.cpp

678

Ok I reworked the patch a bit:

1. Always use "." as the delimiter for the suffix instead of "_" to enable demangling
2. If the label is empty and no suffix passed in from client, use "extracted" after the "." (the demangler needs something after the "." to handle it correctly).
3. Pass in "cold."+Count from the hot cold splitting pass as the suffix, where Count is currently hardwired to 1 but after D53588 can be changed to be a monotonically increasing count of the functions split from a single function.

I will upload the patch momentarily.

I just realized though, that we will still have problems if the hot cold split pass is ever run multiple times, which after D53437 is only an issue with the ThinLTO newPM. I will send a separate patch to fix that, that will need to land before this one.

[xbolva00]

Does the pass groups hot code together? is it in your plans?

[tejohnson]

In D53534#1273164, @xbolva00 wrote:

Does the pass groups hot code together? is it in your plans?

@sebpop, @vsk or @hiraditya can probably answer this. I'm not working on the core hot cold splitting algorithm, just some issues I'm finding as I try it. But presumably the linker should be grouping the split hot functions together and the split cold functions together, when building with -ffunction-sections and PGO which should put them in the .text.hot and .text.unlikely sections, respectively.

[tejohnson]

1. Always use "." as the delimiter for the suffix instead of "_" to enable demangling
2. If the label is empty and no suffix passed in from client, use "extracted" after the "." (the demangler needs something after the "." to handle it correctly).
3. Pass in "cold."+Count from the hot cold splitting pass as the suffix, where Count is currently hardwired to 1 but after D53588 can be changed to be a monotonically increasing count of the functions split from a single function.

Unfortunately change 1) required many test changes.

[sebpop]

The change looks good with some minor changes. Thanks!

lib/Transforms/IPO/HotColdSplitting.cpp

346

Given the commit message: Pass in "cold."+Count
I think the above statement is missing the dot after cold:

/* Suffix */ "cold." + std::to_string(Count));

424

I like this change, makes it very easy to get unique function names.

test/Transforms/HotColdSplit/split-cold-2.ll

7

I was expecting to see an integer appended to "cold" like so "fun.cold.1"

[tejohnson]

lib/Transforms/IPO/HotColdSplitting.cpp

346

Indeed, this was missing the ".", fixed.

test/Transforms/HotColdSplit/split-cold-2.ll

7

Yep, it was passing because of the partial match working. I added the ".1" (which in fact would have caught the oversight you pointed out earlier in the code!).

[tejohnson]

Address comments

[vsk]

In D53534#1273174, @tejohnson wrote:

In D53534#1273164, @xbolva00 wrote:

Does the pass groups hot code together? is it in your plans?

@sebpop, @vsk or @hiraditya can probably answer this. I'm not working on the core hot cold splitting algorithm, just some issues I'm finding as I try it. But presumably the linker should be grouping the split hot functions together and the split cold functions together, when building with -ffunction-sections and PGO which should put them in the .text.hot and .text.unlikely sections, respectively.

The pass currently does not group hot or cold code together. Teresa's explanation is correct -- we'll need linker support to do that. On Darwin I have plans to implement this by adding a section modifier (similar to live_support), as this doesn't require creating a new section or messing with order files.

[sebpop]

ok

include/llvm/Transforms/Utils/CodeExtractor.h

class CodeExtractor {
     // If true, varargs functions can be extracted.
     bool AllowVarArgs;
 
     // Bits of intermediate state computed at various phases of extraction.
     SetVector<BasicBlock *> Blocks;
     unsigned NumExitBlocks = std::numeric_limits<unsigned>::max();
     Type *RetTy;
 
+    // Suffix to use when creating extracted function (appended to the original
+    // function name). If empty, the default is to append "_" + entry block
+    // label to the original function name.
+    StringRef Suffix;
+
   public:
     /// Create a code extractor for a sequence of blocks.
     ///
     /// Given a sequence of basic blocks where the first block in the sequence
     /// dominates the rest, prepare a code extractor object for pulling this
     /// sequence out into its new function. When a DominatorTree is also given,
     /// extra checking and transformations are enabled. If AllowVarArgs is true,
     /// vararg functions can be extracted. This is safe, if all vararg handling
     /// code is extracted, including vastart. If AllowAlloca is true, then
     /// extraction of blocks containing alloca instructions would be possible,
     /// however code extractor won't validate whether extraction is legal.
     CodeExtractor(ArrayRef<BasicBlock *> BBs, DominatorTree *DT = nullptr,
                   bool AggregateArgs = false, BlockFrequencyInfo *BFI = nullptr,
                   BranchProbabilityInfo *BPI = nullptr,
-                  bool AllowVarArgs = false, bool AllowAlloca = false);
+                  bool AllowVarArgs = false, bool AllowAlloca = false,
+                  StringRef Suffix = "");
 
     /// Create a code extractor for a loop body.
     ///
     /// Behaves just like the generic code sequence constructor, but uses the
     /// block sequence of the loop.
     CodeExtractor(DominatorTree &DT, Loop &L, bool AggregateArgs = false,
                   BlockFrequencyInfo *BFI = nullptr,
-                  BranchProbabilityInfo *BPI = nullptr);
+                  BranchProbabilityInfo *BPI = nullptr, StringRef Suffix = "");
 
     /// Perform the extraction, returning the new function.
     ///
     /// Returns zero when called on a CodeExtractor instance where isEligible
     /// returns false.
     Function *extractCodeRegion();
 
     /// Test whether this code extractor is eligible.

lib/Transforms/IPO/HotColdSplitting.cpp

 HotColdSplitting::extractColdRegion(const SmallVectorImpl<BasicBlock *> &Region,
                                     DominatorTree *DT, BlockFrequencyInfo *BFI,
                                     OptimizationRemarkEmitter &ORE) {
   assert(!Region.empty());
   LLVM_DEBUG(for (auto *BB : Region)
           llvm::dbgs() << "\nExtracting: " << *BB;);
 
   // TODO: Pass BFI and BPI to update profile information.
-  CodeExtractor CE(Region, DT);
+  CodeExtractor CE(Region, DT, /* AggregateArgs */ false, /* BFI */ nullptr,
+                   /* BPI */ nullptr, /* AllowVarArgs */ false,
+                   /* AllowAlloca */ false, /* Suffix */ ".cold");
 
   SetVector<Value *> Inputs, Outputs, Sinks;
   CE.findInputsOutputs(Inputs, Outputs, Sinks);
 
   // Do not extract regions that have live exit variables.
   if (Outputs.size() > 0)
     return nullptr;
 
   Function *OrigF = Region[0]->getParent();
if (PSI->isColdBB(BB, BFI) || !HotBlocks.count(BB)) {
       if (ExitColdRegion) {
         // Do not outline a region with only one block.
         if (ValidColdRegion.size() == 1)
           continue;
 
         ++NumColdSESEFound;
         ValidColdRegion.push_back(ExitColdRegion);
         // Candidate for outlining. FIXME: Continue outlining.
         return extractColdRegion(ValidColdRegion, DT, BFI, ORE);
       }
     }
   }
   return nullptr;
 }
 
 bool HotColdSplitting::run(Module &M) {
   for (auto &F : M) {

lib/Transforms/Utils/CodeExtractor.cpp

buildExtractionBlockSet(ArrayRef<BasicBlock *> BBs, DominatorTree *DT,
   }
 
   return Result;
 }
 
 CodeExtractor::CodeExtractor(ArrayRef<BasicBlock *> BBs, DominatorTree *DT,
                              bool AggregateArgs, BlockFrequencyInfo *BFI,
                              BranchProbabilityInfo *BPI, bool AllowVarArgs,
-                             bool AllowAlloca)
+                             bool AllowAlloca, StringRef Suffix)
     : DT(DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
       BPI(BPI), AllowVarArgs(AllowVarArgs),
-      Blocks(buildExtractionBlockSet(BBs, DT, AllowVarArgs, AllowAlloca)) {}
+      Blocks(buildExtractionBlockSet(BBs, DT, AllowVarArgs, AllowAlloca)),
+      Suffix(Suffix) {}
 
 CodeExtractor::CodeExtractor(DominatorTree &DT, Loop &L, bool AggregateArgs,
                              BlockFrequencyInfo *BFI,
-                             BranchProbabilityInfo *BPI)
+                             BranchProbabilityInfo *BPI, StringRef Suffix)
     : DT(&DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
       BPI(BPI), AllowVarArgs(false),
       Blocks(buildExtractionBlockSet(L.getBlocks(), &DT,
                                      /* AllowVarArgs */ false,
-                                     /* AllowAlloca */ false)) {}
+                                     /* AllowAlloca */ false)),
+      Suffix(Suffix) {}
 
 /// definedInRegion - Return true if the specified value is defined in the
 /// extracted region.
 static bool definedInRegion(const SetVector<BasicBlock *> &Blocks, Value *V) {
   if (Instruction *I = dyn_cast<Instruction>(V))
     if (Blocks.count(I->getParent()))
       return true;
   return false;
Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   }
   FunctionType *funcType =
                   FunctionType::get(RetTy, paramTy,
                                     AllowVarArgs && oldFunction->isVarArg());
 
   // Create the new function
   Function *newFunction = Function::Create(
       funcType, GlobalValue::InternalLinkage, oldFunction->getAddressSpace(),
-      oldFunction->getName() + "_" + header->getName(), M);
+      Suffix.empty() ? oldFunction->getName() + "_" + header->getName()
+                     : oldFunction->getName() + Suffix,
+      M);
   // If the old function is no-throw, so is the new one.
   if (oldFunction->doesNotThrow())
     newFunction->setDoesNotThrow();
 
   // Inherit the uwtable attribute if we need to.
   if (oldFunction->hasUWTable())
     newFunction->setHasUWTable();
 

test/Transforms/HotColdSplit/split-cold-2.ll

 ; RUN: opt -hotcoldsplit -pass-remarks=hotcoldsplit -S < %s 2>&1 | FileCheck %s
 ; RUN: opt -passes=hotcoldsplit -pass-remarks=hotcoldsplit -S < %s 2>&1 | FileCheck %s
 
 ; Make sure this compiles. This test used to fail with an invalid phi node: the
 ; two predecessors were outlined and the SSA representation was invalid.
 
-; CHECK: remark: <unknown>:0:0: fun split cold code into fun_if.else
+; CHECK: remark: <unknown>:0:0: fun split cold code into fun.cold
 ; CHECK-LABEL: @fun
 ; CHECK: codeRepl:
-; CHECK-NEXT: call void @fun_if.else
+; CHECK-NEXT: call void @fun.cold
 
 define void @fun() {
 entry:
   br i1 undef, label %if.then, label %if.else
 
 if.then:
   ret void
 

test/Transforms/HotColdSplit/split-out-dbg-val-of-arg.ll

 ; RUN: opt -hotcoldsplit -S < %s | FileCheck %s
 
-; CHECK-LABEL: define {{.*}}@foo_if.end
+; CHECK-LABEL: define {{.*}}@foo.cold
 ; CHECK-NOT: llvm.dbg.value
 
 define void @foo(i32 %arg1) !dbg !6 {
 entry:
   %var = add i32 0, 0, !dbg !11
   br i1 undef, label %if.then, label %if.end, !dbg !12
 
 if.then:                                          ; preds = %entry