Use .text.unlikely and .text.eh prefixes for MachineBasicBlock sections.

[snehasish]

Summary

Instead of adding a ".unlikely" or ".eh" suffix for machine basic blocks,
this change updates the behaviour to use an appropriate prefix
instead. This allows lld to group basic block sections together
when -z,keep-text-section-prefix is specified and matches the behaviour
observed in gcc.

[tmsriram]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

889

This looks good. This should also be done for ".eh", .text.eh must be the prefix. Further, the parent name must only be appended for unlikely and eh sections as there is only one such section for each function and using the function name guarantees that.

For other sections, there is no need to append the parent name. Either it should be .text (or .text.hot) or .text.<MBBSymbolName> when UniqueBBSectionNames is used. Does this make sense? Thanks for working on this.

[snehasish]

Use a ".text.eh" prefix instead of ".eh" suffix for exception machine basic block sections.
Update the basic-block-sections-clusters-eh test.

[snehasish]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

889

Done. I've updated the diff to use the ".text.eh" prefix as suggested.

The behaviour for the other sections is actually unchanged. We use the name of the parent section and not the parent. I just removed the static_cast to MCSectionELF* before calling getName() since it is unnecessary.

[tmsriram]

This is a small change and looks good to me. I think this is consistent with what the linker expects for unlikely sections, so I am approving it. Adding @efriedma just to be sure. Please wait for a response before pushing.

[tmsriram]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

881

Nit, use braces around the else too.

[snehasish]

Addressed review comment.

[efriedma]

LGTM. It looks like gcc also emits .text.unlikely.foo

[snehasish]

Thanks for the reviews @eli.friedman @tmsriram.
Can someone go ahead and push this this for me since I don't have commit access?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

MCSection *TargetLoweringObjectFileELF::getSectionForConstant(
   return DataRelROSection;
 }
 
 /// Returns a unique section for the given machine basic block.
 MCSection *TargetLoweringObjectFileELF::getSectionForMachineBasicBlock(
     const Function &F, const MachineBasicBlock &MBB,
     const TargetMachine &TM) const {
   assert(MBB.isBeginSection() && "Basic block does not start a section!");
-  SmallString<128> Name;
-  Name =
-      (static_cast<MCSectionELF *>(MBB.getParent()->getSection()))->getName();
   unsigned UniqueID = MCContext::GenericSectionID;
 
-  switch (MBB.getSectionID().Type) {
-    // Append suffixes to represent special cold and exception sections.
-  case MBBSectionID::SectionType::Exception:
-    Name += ".eh";
-    break;
-  case MBBSectionID::SectionType::Cold:
-    Name += ".unlikely";
-    break;
-  // For regular sections, either use a unique name, or a unique ID for the
-  // section.
-  default:
+  // 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 += MBB.getParent()->getName();
+  } else if (MBB.getSectionID() == MBBSectionID::ExceptionSectionID) {
+    Name += ".text.eh.";
+    Name += MBB.getParent()->getName();
+  } else {
+    Name += MBB.getParent()->getSection()->getName();
     if (TM.getUniqueBBSectionNames()) {
       Name += ".";
       Name += MBB.getSymbol()->getName();
-    } else
+    } else {
       UniqueID = NextUniqueID++;
-    break;
+    }
   }
 
   unsigned Flags = ELF::SHF_ALLOC | ELF::SHF_EXECINSTR;
   std::string GroupName = "";
   if (F.hasComdat()) {
     Flags |= ELF::SHF_GROUP;
     GroupName = F.getComdat()->getName().str();
   }
   return getContext().getELFSection(Name, ELF::SHT_PROGBITS, Flags,
                                     0 /* Entry Size */, GroupName, UniqueID,
                                     nullptr);
 }
 
 static MCSectionELF *getStaticStructorSection(MCContext &Ctx, bool UseInitArray,

llvm/test/CodeGen/X86/basicblock-sections-clusters-branches.ll

 ; LINUX-SECTIONS1-LABEL:  foo:
 ; LINUX-SECTIONS1:    jne a.BB.foo
 ; LINUX-SECTIONS1-NOT:    {{jne|je|jmp}}
 ; LINUX-SECTIONS1-LABEL:  # %bb.2:
 ; LINUX-SECTIONS1:    jmp raa.BB.foo
 ; LINUX-SECTIONS1:    .section        .text.foo,"ax",@progbits,unique,1
 ; LINUX-SECTIONS1-LABEL:  a.BB.foo:
 ; LINUX-SECTIONS1:    jmp raa.BB.foo
-; LINUX-SECTIONS1:    .section        .text.foo.unlikely,"ax",@progbits
+; LINUX-SECTIONS1:    .section        .text.unlikely.foo,"ax",@progbits
 ; LINUX-SECTIONS1-LABEL:  raa.BB.foo:
 
 ; LINUX-SECTIONS2:    .section        .text.foo,"ax",@progbits
 ; LINUX-SECTIONS2-LABEL:  foo:
 ; LINUX-SECTIONS2:    jne a.BB.foo
 ; LINUX-SECTIONS2-NOT:    {{jne|je|jmp}}
 ; LINUX-SECTIONS2-LABEL:  # %bb.2:
 ; LINUX-SECTIONS2:    jmp .LBB0_3
 ; LINUX-SECTIONS2:    .section        .text.foo,"ax",@progbits,unique,1
 ; LINUX-SECTIONS2:    a.BB.foo:
 ; LINUX-SECTIONS2-NOT:    {{jne|je|jmp}}
 ; LINUX-SECTIONS2:    .LBB0_3:

llvm/test/CodeGen/X86/basicblock-sections-clusters-eh.ll

 ; LINUX-SECTIONS1-NOT:    .section
 ; LINUX-SECTIONS1-LABEL:  .LBB0_4:
 ; LINUX-SECTIONS1-NOT:    .section
 ; LINUX-SECTIONS1-LABEL:  .LBB0_5:
 ; LINUX-SECTIONS1-NOT:    .section
 ; LINUX-SECTIONS1-LABEL:  .LBB0_6:
 ; LINUX-SECTIONS1:    .section  .text.main,"ax",@progbits,unique,1
 ; LINUX-SECTIONS1-LABEL:  al.BB.main:
-; LINUX-SECTIONS1:    .section  .text.main.eh,"ax",@progbits
+; LINUX-SECTIONS1:    .section  .text.eh.main,"ax",@progbits
 ; LINUX-SECTIONS1-LABEL:  l.BB.main:
 ; LINUX-SECTIONS1-NOT:    .section
 ; LINUX-SECTIONS1-LABEL:  .LBB0_3:
 ; LINUX-SECTIONS1-NOT:    .section
 ; LINUX-SECTIONS1:    .section  .text.main,"ax",@progbits
 ; LINUX-SECTIONS1-LABEL:  .Lfunc_end0
 
 

llvm/test/CodeGen/X86/basicblock-sections-clusters.ll

 ; LINUX-SECTIONS1-LABEL:  foo:
 ; LINUX-SECTIONS1-NOT:    .section
 ; LINUX-SECTIONS1-LABEL:  # %bb.2:
 ; LINUX-SECTIONS1:    .section        .text.foo,"ax",@progbits,unique,1
 ; LINUX-SECTIONS1-LABEL:  a.BB.foo:
 ; LINUX-SECTIONS1-LABEL:  .Ltmp0:
 ; LINUX-SECTIONS1-NEXT:   .size a.BB.foo, .Ltmp0-a.BB.foo
 ; LINUX-SECTIONS1-NOT:    .section
-; LINUX-SECTIONS1:    .section        .text.foo.unlikely,"ax",@progbits
+; LINUX-SECTIONS1:    .section        .text.unlikely.foo,"ax",@progbits
 ; LINUX-SECTIONS1-LABEL:  raa.BB.foo:
 ; LINUX-SECTIONS1:      .section  .text.foo,"ax",@progbits
 ; LINUX-SECTIONS1-LABEL:  .Lfunc_end0:
 ; LINUX-SECTIONS1-NEXT:   .size foo, .Lfunc_end0-foo
 
 ; LINUX-SECTIONS2:    .section        .text.foo,"ax",@progbits
 ; LINUX-SECTIONS2-NOT:    .section
 ; LINUX-SECTIONS2-LABEL:  foo:

llvm/test/CodeGen/X86/basicblock-sections-cold.ll

 
 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.r.BB._Z3bazb,"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._Z3bazb.unlikely,"ax",@progbits
+; LINUX-SECTIONS: .section  .text.unlikely._Z3bazb,"ax",@progbits
 ; LINUX-SECTIONS: r.BB._Z3bazb:
 ; LINUX-SECTIONS-NOT: .section        .text._Z3bazb.rr.BB._Z3bazb,"ax",@progbits,unique
 ; LINUX-SECTIONS: .LBB0_2:
 ; LINUX-SECTIONS: .size   _Z3bazb, .Lfunc_end{{[0-9]}}-_Z3bazb