[PATCH] D93876: Do not implicitly turn on function sections with basic block sections.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 7 12:22:17 PST 2021
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
More information about the llvm-commits
mailing list