[llvm] Add -bbsections-cold-text-prefix to emit cold clusters to a different section.

[snehasish]

Summary

This change adds an option to basic block sections to allow cold
clusters to be assigned a custom text prefix. With a custom prefix such
as ".text.split." (D87840), lld can place them in a separate output section.
The benefits are -

• Empirically shown to improve icache and itlb metrics by 3-5% (absolute) compared to placing split parts in .text.unlikely.
• Mitigates against poor profiles, eg samplePGO profiles used with the machine function splitter. Optimizations such as hugepage remapping can make different decisions at the section granularity.
• Enables section granularity hotness monitoring (checking on the decisions made during compilation vs sample data from production).

[MaskRay]

lld/ELF/Writer.cpp

134 ↗

(On Diff #292405)

I think I will need to read your first LLVM patch to get some basic understanding of machine function splitter. I'd suggest you split the lld patch from the codegen one.

For your future lld patch: why can't the split sections be placed in .test.cold? This just affects how -z keep-text-section-prefix groups input sections into output sections. With the additional element, there may be an output section .text.split but if its purpose is similar to the others I am not sure you need it.

[snehasish]

Drop lld/ELF/Writer.cpp changes.

• Moved the changes to D87840.

lld/ELF/Writer.cpp

134 ↗

(On Diff #292405)

Thanks, here's a link to the RFC for your reference: https://groups.google.com/g/llvm-dev/c/RUegaMg-iqc/m/wFAVxa6fCgAJ
I've split out the LLD change as a separate patch: D87840

I'll add the discussion for the rationale behind a new output section in the D87840 (assuming that you mean ".text.unlikely" instead of ".test.cold").

[tmsriram]

lld/ELF/Writer.cpp

134 ↗

(On Diff #292405)

A couple of thoughts on this which you could help clarify:

• Currently, known cold code is put into .text.unlikely and lukewarm stuff is put into .text.
• Hugepage mappers that I am aware of avoid mapping .text.unlikely into hugepages for max hugepage itlb utilization.
• For function splitting, when you play with the thresholds more there is a good chance you are going to end up splitting code that is lukewarm and .text.unlikely is not the ideal place for it.
• In which case, such code can be put into .text itself. Is there a good reason to put this into .text.split? Does having a new output section give you more leverage on how to manage the mapping of such code? I think this is the part that is not fully clear to me. Thanks.

[snehasish]

Rebase and update git commit message.

[snehasish]

ping @tmsriram @MaskRay

[tmsriram]

This LGTM after I understood the rationale:

1. It allows fine-grained control of cold split part placement for spatial locality and either mapping to or excluding from huge pages. This becomes very relevant as we play with the coldness thresholds where lukewarm parts could be split from hot function bodies. For these reasons, reusing an existing prefix is not ideal.
2. There is a precedent with ".text.unknown" which has similar motivations. The lld patch for this is also rather simple.

[MaskRay]

llvm/lib/CodeGen/BasicBlockSections.cpp

82

Worth a comment why the toggle exists.

84

The description usually does not end with a full stop

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

69

This is probably better declared in a header and named something with BB as a prefix, otherwise it can lead to confusion that this applies to the generic .text.unlikely

llvm/test/CodeGen/X86/basic-block-sections-cold.ll

39

If -NEXT: applies please add it.

[MaskRay]

[llvm]Add an option to emit cold clusters to a different section.

Instead of saying "Add an option", "Add -bbsections-cold-prefix" will convey more information. (Not rarely I grep logs for when an option was introduced. It is helpful if I can find the option name in the message)

[MaskRay]

llvm/test/CodeGen/X86/basic-block-sections-cold.ll

39

I think it'd be better to check .LBB0_2: and .LBB_END0_2: as well to enhance the test.

[snehasish]

Document flag, tighten test, rename var and option for clarity.

• Flag name and var name: bbsection-cold-text-prefix - BBSectionColdTextPrefix
• Add comment about why this option is useful.
• Update test to be more rigourous.
• Rebase.

[snehasish]

Add another check for the test.

[snehasish]

PTAL, thanks!

[snehasish]

Update git commit message to specify option.

[MaskRay]

LGTM.

llvm/lib/CodeGen/BasicBlockSections.cpp

82

I know your intention is that in practice users should specify .text.split. , however, the comment is a bit unclear that normally users should not use .text.unlikely.

llvm/test/CodeGen/X86/basic-block-sections-cold.ll

40

I usually indent instructions to emphasize that they are in the region covered by a label (like _Z3bazb.cold:)

[snehasish]

Update comment, test.

• Specify how users can use the flag and take advantage of keep-text-section-prefix.
• Indent the instructions in check for clarity.

[snehasish]

Thanks for the review.

llvm/lib/CodeGen/BasicBlockSections.cpp

82

Good idea, documented the prefix and relevant lld flag.

llvm/lib/CodeGen/BasicBlockSections.cpp

 #include "llvm/Target/TargetMachine.h"
 
 using llvm::SmallSet;
 using llvm::SmallVector;
 using llvm::StringMap;
 using llvm::StringRef;
 using namespace llvm;
 
+cl::opt<std::string> ColdSectionTextPrefix(
+    "bbsections-cold-prefix",
+    cl::desc("The text prefix to use for cold basic block clusters."),
+    cl::init(".text.unlikely."), cl::Hidden);
+
 namespace {
 
 // This struct represents the cluster information for a machine basic block.
 struct BBClusterInfo {
   // MachineBasicBlock ID.
   unsigned MBBNumber;
   // Cluster ID this basic block belongs to.
   unsigned ClusterID;

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include <cassert>
 #include <string>
 
 using namespace llvm;
 using namespace dwarf;
 
+extern cl::opt<std::string> ColdSectionTextPrefix;
+
 static void GetObjCImageInfo(Module &M, unsigned &Version, unsigned &Flags,
                              StringRef &Section) {
   SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
   M.getModuleFlagsMetadata(ModuleFlags);
 
   for (const auto &MFE: ModuleFlags) {
     // Ignore flags with 'Require' behaviour.
     if (MFE.Behavior == Module::Require)
MCSection *TargetLoweringObjectFileELF::getSectionForMachineBasicBlock(
 
   // For cold sections use the .text.unlikely prefix along with the parent
   // function name. All cold blocks for the same function go to the same
   // section. Similarly all exception blocks are grouped by symbol name
   // under the .text.eh prefix. For regular sections, we either use a unique
   // name, or a unique ID for the section.
   SmallString<128> Name;
   if (MBB.getSectionID() == MBBSectionID::ColdSectionID) {
-    Name += ".text.unlikely.";
+    Name += ColdSectionTextPrefix;
     Name += MBB.getParent()->getName();
   } else if (MBB.getSectionID() == MBBSectionID::ExceptionSectionID) {
     Name += ".text.eh.";
     Name += MBB.getParent()->getName();
   } else {
     Name += MBB.getParent()->getSection()->getName();
     if (TM.getUniqueBasicBlockSectionNames()) {
       Name += ".";

llvm/test/CodeGen/X86/basic-block-sections-cold.ll

 ; Check if basic blocks that don't get unique sections are placed in cold sections.
 ; Basic block with id 1 and 2 must be in the cold section.
 ; RUN: echo '!_Z3bazb' > %t
 ; RUN: echo '!!0' >> %t
-; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t -unique-basic-block-section-names -bbsections-cold-prefix=".text.split." | FileCheck %s -check-prefix=LINUX-SPLIT
 
-define void @_Z3bazb(i1 zeroext) nounwind {
-  %2 = alloca i8, align 1
-  %3 = zext i1 %0 to i8
-  store i8 %3, i8* %2, align 1
-  %4 = load i8, i8* %2, align 1
-  %5 = trunc i8 %4 to i1
-  br i1 %5, label %6, label %8
-
-6:                                                ; preds = %1
-  %7 = call i32 @_Z3barv()
-  br label %10
-
-8:                                                ; preds = %1
-  %9 = call i32 @_Z3foov()
-  br label %10
+define void @_Z3bazb(i1 zeroext %0) nounwind {
+  br i1 %0, label %2, label %4
 
-10:                                               ; preds = %8, %6
+2:                                                ; preds = %1
+  %3 = call i32 @_Z3barv()
+  br label %6
+
+4:                                                ; preds = %1
+  %5 = call i32 @_Z3foov()
+  br label %6
+
+6:                                                ; preds = %2, %4
   ret void
 }
 
 declare i32 @_Z3barv() #1
 
 declare i32 @_Z3foov() #1
 
 ; LINUX-SECTIONS: .section        .text._Z3bazb,"ax",@progbits
 ; LINUX-SECTIONS: _Z3bazb:
 ; Check that the basic block with id 1 doesn't get a section.
 ; LINUX-SECTIONS-NOT: .section        .text._Z3bazb._Z3bazb.1,"ax",@progbits,unique
 ; Check that a single cold section is started here and id 1 and 2 blocks are placed here.
 ; LINUX-SECTIONS: .section  .text.unlikely._Z3bazb,"ax",@progbits
 ; LINUX-SECTIONS: _Z3bazb.cold:
 ; LINUX-SECTIONS-NOT: .section        .text._Z3bazb._Z3bazb.2,"ax",@progbits,unique
 ; LINUX-SECTIONS: .LBB0_2:
 ; LINUX-SECTIONS: .size   _Z3bazb, .Lfunc_end{{[0-9]}}-_Z3bazb
+
+; LINUX-SPLIT: .section .text.split._Z3bazb,"ax",@progbits
+; LINUX-SPLIT: _Z3bazb.cold: