Page MenuHomePhabricator

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

Authored by tmsriram on Mon, Dec 28, 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.Mon, Dec 28, 3:43 PM
tmsriram requested review of this revision.Mon, Dec 28, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Dec 28, 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
932

Can we add const here?

939

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

941

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.Wed, Jan 6, 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.Thu, Jan 7, 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
933–947

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–21

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.Thu, Jan 7, 12:22 PM