This is an archive of the discontinued LLVM Phabricator instance.

Do not implicitly turn on function sections with basic block sections.
ClosedPublic

Authored by tmsriram on Dec 28 2020, 3:43 PM.

Details

Summary

Basic block sections enables function sections implicitly, this is not needed and is inefficient with "=list" option.

We had basic block sections enable function sections implicitly in clang. This is particularly inefficient with "=list" option as it places functions that do not have any basic block sections in separate sections. This causes unnecessary object file overhead for large applications.

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Further, there was an inconistent behavior with llc as llc was not turning on function sections by default. This patch makes llc and clang consistent and tests are added to check the new behavior.

Diff Detail

Event Timeline

tmsriram created this revision.Dec 28 2020, 3:43 PM
tmsriram requested review of this revision.Dec 28 2020, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 28 2020, 3:43 PM

Can we add a test with the =list option to verify that functions not requiring basic block sections are not emitted in unique sections?
Or maybe we can use the existing basic-block-sections-list.ll by adding a new invocation without -function-sections.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
931

Can we add const here?

938

It makes it much more clear if we add an inline comment like /* EmitUniqueSection */ true here.

940

Maybe we can add a message to the assert.

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

tmsriram updated this revision to Diff 315053.Jan 6 2021, 11:06 PM
tmsriram marked 3 inline comments as done.

Address reviewer comments. Extend test to check for no function sections.

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

But we want the .text section to be unique because we want reorder functions at link time. Does that make sense?

Can we add a test with the =list option to verify that functions not requiring basic block sections are not emitted in unique sections?
Or maybe we can use the existing basic-block-sections-list.ll by adding a new invocation without -function-sections.

Good suggestion, I extended the test.

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

But we want the .text section to be unique because we want reorder functions at link time. Does that make sense?

Somewhat - but if the user wants reordering, wouldn't they want -ffunction-sections too?

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"?

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

But we want the .text section to be unique because we want reorder functions at link time. Does that make sense?

Somewhat - but if the user wants reordering, wouldn't they want -ffunction-sections too?

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 :)

dblaikie accepted this revision.Jan 7 2021, 12:22 PM

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)

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
932–946

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)

llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
10

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?

llvm/test/CodeGen/X86/basic-block-sections-mir-parse.mir
125

Similarly here - somehow this changed ended up enabling unique-section-names when it wasn't enabled prior to this patch?

llvm/test/DebugInfo/X86/basic-block-sections_1.ll
19–23

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?

This revision is now accepted and ready to land.Jan 7 2021, 12:22 PM
tmsriram updated this revision to Diff 323520.Feb 12 2021, 10:06 PM
tmsriram marked 4 inline comments as done.
tmsriram added inline comments.Feb 12 2021, 10:06 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
932–946

Thanks for the refactoring idea. I took the liberty to put it in this patch as it motivates the refactoring need.

llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
10

llc did not enable function sections implicitly with basic block sections. This patch exposed that problem. With this patch, that gets fixed.

llvm/test/CodeGen/X86/basic-block-sections-mir-parse.mir
125

Same problem here. clang implictly turned on function sections when basic block sections was requested but not llc. This got exposed here. Great catch!

llvm/test/DebugInfo/X86/basic-block-sections_1.ll
19–23

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.

dblaikie added inline comments.Feb 15 2021, 1:47 PM
llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
10

Not sure I follow - the patch description says "Do not implicitly turn on function sections with basic block sections." - which seems in contradiction to the comment here about llc not enabling function sections implicitly with bb sections and this patch fixing it, and this test now having function sections when it didn't before.

tmsriram added inline comments.Feb 15 2021, 4:00 PM
llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
10

TLDR; foo requires basic block sections so the entry block also gets a unique section via the newly added function.

Alright, let me explain in more detail. Previously, llc did not turn on function sections with basic block sections. So, even though bbsections was enabled, function foo's entry basic block did not get its own section. That part is clear. Just for distinction, previously clang did enable function sections implicitly with basic block sections. So, the same test compiled with clang instead of llc would have seen a unique section for the entry basic block of foo.

Now, with basic block sections enabled, we do not create a separate section for a function that does not need basic block sections, this is with both llc and clang. However, if a function needs basic block sections, like foo in this case, we do create a unique section for its entry basic block too with the newly added function "getUniqueSectionForFunction" in TargetLoweringObjectFileImpl.cpp.

For the counter example, please test basic-block-sections-list.ll. There, function zip does not need bb sections. So, it will not be placed in a unique function section either and the test explicitly checks for this.

dblaikie added inline comments.Feb 16 2021, 12:19 PM
llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
10

Oh, this is two different changes in one patch, then. (generally anything with a clang change and an llvm change this is the case, but sometimes they're intrinsically linked/can't be separated (eg: an API change))

So, step 1) Change LLVM to generate unique sections for functions that need BB sections (that causes this test and others to change) - this has an LLVM effect, and is a feature addition there - but has no effect on the Clang use case, because Clang was/is currently enabling function sections by default when bb sections is used, so the sections don't get any more granular because they're already highly granular

then (2), once (1) is supported, removes the function-section implication in Clang now that it's not needed to support bb sections (since bb sections can now create its own unique sections on an as-needed basis, rather than needing the shotgun approach of enabling them for all functions)

Yeah?

Generally would be helpful if these sort of things were done in separate reviews - I think it'd have made it clear where the behavior is changing (admittedly, if I'd been a bit more clear-headed when reading this, it would've been clear enough that this is changing a bunch of LLVM things, so it's not only a change to Clang's defaults).

But I think it's reviewed enough here - but please commit it as two separate patches (hopefully that'll help by having more narrow patch descriptions that'll make it clearer what's happening in each step).

tmsriram marked an inline comment as done.Feb 16 2021, 12:29 PM
tmsriram added inline comments.
llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll
10

Yeah, that works! I can split it into two patches like you described and add a patch i of 2 to make it clear. Thanks for taking a look!

This revision was automatically updated to reflect the committed changes.
tmsriram marked an inline comment as done.