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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added inline comments.


================
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,
----------------
dblaikie wrote:
> 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)
Thanks for the refactoring idea.  I  took the liberty to put it in this patch as it motivates the refactoring need.


================
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:
----------------
dblaikie wrote:
> 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?
llc did not enable function sections implicitly with basic block sections.  This patch exposed that problem.  With this patch, that gets fixed.


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-mir-parse.mir:125
 
+# CHECK: .section	.text._Z3foob,"ax", at progbits
 # CHECK: _Z3foob:
----------------
dblaikie wrote:
> Similarly here - somehow this changed ended up enabling unique-section-names when it wasn't enabled prior to this patch?
Same problem here.  clang implictly turned on function sections when basic block sections was requested but not llc.  This got exposed here. Great catch!


================
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"
----------------
dblaikie wrote:
> 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?
I have this on my to-do and I too noticed it much later.  Basically, like you observe, -funique-section-names can be used to decide basic block section names too and a separate option is not needed.  Further, some munging of the basic block section names is required.  I will send another patch that cleans this up.


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

https://reviews.llvm.org/D93876