[PATCH] D93876: Do not implicitly turn on function sections with basic block sections.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D93876#2484997 <https://reviews.llvm.org/D93876#2484997>, @tmsriram wrote:

> In D93876#2484989 <https://reviews.llvm.org/D93876#2484989>, @dblaikie wrote:
>
>> I guess if they use bb-sections but not function-sections that assumes selective bb-sections would apply to all the hot functions? So that lets the hot code be reordered, and all the functions that aren't bb-split would go in one big .text section and be considered "not hot"?
>
> Yep, +1 on the last point. That is what I was trying to say but clearly failed to communicate as well :)

Cool, thanks for walking me through it!

Some minor/mechanical and broader points, but patch generally makes sense to me. (might want to double-check with @rahmanl that their feedback has been resolved too)



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:933-947
+  unsigned Flags = getELFSectionFlags(Kind);
+  const MCSymbolELF *LinkedToSym = getLinkedToSymbol(&F, TM);
+  if (LinkedToSym)
+    Flags |= ELF::SHF_LINK_ORDER;
+
+  MCSectionELF *Section = selectELFSectionForGlobal(
+      getContext(), &F, Kind, getMangler(), TM, /* EmitUniqueSection */ true,
----------------
Rather than copying much of SelectSectionForGlobal, perhaps the common parts could be refactored into a shared function?

```

MCSection *TargetLoweringObjectFileELF::SelectSectionForGlobal(
      const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
    unsigned Flags = getELFSectionFlags(Kind);
  
    // If we have -ffunction-section or -fdata-section then we should emit the
    // global value to a uniqued section specifically for it.
    bool EmitUniqueSection = false;
    if (!(Flags & ELF::SHF_MERGE) && !Kind.isCommon()) {
      if (Kind.isText())
        EmitUniqueSection = TM.getFunctionSections();
      else
        EmitUniqueSection = TM.getDataSections();
    }
    EmitUniqueSection |= GO->hasComdat();
    return SelectSectionForGlobal(GO, Kind, TM, EmitUniqueSection);
}
MCSection *TargetLoweringObjectFileELF::SelectSectionForGlobal(
      const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM, bool EmitUniqueSection) const {
    unsigned Flags = getELFSectionFlags(Kind);

    const MCSymbolELF *LinkedToSym = getLinkedToSymbol(GO, TM);
    if (LinkedToSym) {
      EmitUniqueSection = true;
      Flags |= ELF::SHF_LINK_ORDER;
    }
  
    MCSectionELF *Section = selectELFSectionForGlobal(
        getContext(), GO, Kind, getMangler(), TM, EmitUniqueSection, Flags,
        &NextUniqueID, LinkedToSym);
    assert(Section->getLinkedToSymbol() == LinkedToSym);
    return Section;
}
MCSection *TargetLoweringObjectFileELF::getUniqueSectionForFunction(
    const Function &F, const TargetMachine &TM) const {
  return SelectSectionForGlobal(F, SectionKind::getText(), TM, /*EmitUniqueSection = */true);
}
```

If you like/I tend to commit that sort of split potentially as a separate pre-emptive commit (no further review required, tag such a commit as "NFC" (No Functional Change)) enabling the sharing of functionality you desire, then the functional commit can be smaller/more targeted. (no worries either way - there's always some tradeoff, not doing too much "unmotivated" refactoring (where the code looks too weird without the new/future/intended use case) but splitting a function like this is pretty benign on that spectrum)


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll:10
 ; CHECK:         .text
+; CHECK:         .section .text.foo,"ax", at progbits
 ; CHECK-LABEL: foo:
----------------
Why did this test change? It looks like it ended up with unique section names + function-sections enabled, but bb-unique-section-names not enabled? But that wasn't the case prior to this patch?


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-mir-parse.mir:125
 
+# CHECK: .section	.text._Z3foob,"ax", at progbits
 # CHECK: _Z3foob:
----------------
Similarly here - somehow this changed ended up enabling unique-section-names when it wasn't enabled prior to this patch?


================
Comment at: llvm/test/DebugInfo/X86/basic-block-sections_1.ll:19-21
+; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi._Z3fooi.__part.1"
+; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi._Z3fooi.__part.2"
+; BB-SECTIONS-NEXT: [{{.*}}) ".text._Z3fooi._Z3fooi.__part.3"
----------------
What happened here? I guess the function was in a function section using a unique name (.text._Z3fooi) and then basic block sections were made out of that, by appending a unique suffix for the function, plus unique suffixes for each part?

Is the extra unique suffix needed? It may hurt binary size somewhat. (while I realize at least in Google we don't enable unique section names - still, be good to make this a bit tidier/less redundant)


Also, it seems like bb-section unique names is a separate flag from the "unique section names" flag? That doesn't seem quite correct to me - if the user says -fbasic-block-sections=all -funique-section-names, seems to me they should get unique section names, without the need to specify another flag. It doesn't seem meaningful/valuable to be able to have unique function section names and non-unique bb section names, or the other way around - the need for unique section names is usually due to a limitation in the user's toolchain (assembler, linker, etc) that can't handle non-unique section names, so having uniqueness in one area, but not overall doesn't meet that need. Be great to remove the bb-sections-uniqueness flag and use the general one generally.

Oh, and given that 'cold', 'eh', etc, work as suffixes, would 'part' work as a suffix, without the '__'? Maybe 'bb' would be descriptive?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93876/new/

https://reviews.llvm.org/D93876