Page MenuHomePhabricator

Propeller: Clang options for basic block sections
Needs ReviewPublic

Authored by tmsriram on Sep 25 2019, 1:29 PM.

Details

Summary

Options for Basic Block Sections, enabled in D68063 and D73674.

This patch adds clang options -fbasicblock-sections={all,<filename>,labels,none} and -funique-bb-section-names. LLVM Support for basic block sections is already enabled.

+ -fbasicblock-sections={all, <file>, labels, none} : Enables/Disables basic block sections for all or a subset of basic blocks. "labels" only enables basic block symbols.
+ -funique-bb-section-names: Enables unique section names for basic block sections, disabled by default.

Diff Detail

Event Timeline

tmsriram created this revision.Sep 25 2019, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 1:29 PM
mehdi_amini added inline comments.Sep 25 2019, 3:26 PM
clang/include/clang/Basic/CodeGenOptions.def
53

Nit: indentation is off here.

367

Can you add a doc here? (possibly referring to somewhere else if it is extensively documented elsewhere?)

clang/include/clang/Driver/Options.td
1974

Is the "labels" options dependent/related to the previous -fpropeller-label one?

clang/lib/CodeGen/CGDeclCXX.cpp
348 ↗(On Diff #221825)

Is this related to these new option? It this change the behavior of -ffunction-sections?

clang/lib/CodeGen/CodeGenModule.cpp
1032 ↗(On Diff #221825)

I agree with the improvement, but as nit this isn't related to the current patch or even in a function you're otherwise touching. (it creates an extra hunk in the review)

1100 ↗(On Diff #221825)

What happens in case of conflict? (maybe clarify in the comment)

MaskRay added inline comments.Sep 26 2019, 5:12 AM
clang/lib/CodeGen/BackendUtil.cpp
445
if (S.consume_front("!")) {
  if (S.empty())
    ...
  else
    ... 
}
clang/lib/Driver/ToolChains/Gnu.cpp
631 ↗(On Diff #221825)

This check is overly constrained. Some systems default to use lld (e.g. installed at /usr/bin/ld). I suggest removing this check.

640 ↗(On Diff #221825)

Why --no-call-graph-profile-sort?

642 ↗(On Diff #221825)

This will silently ignore user specified -z keep-text-section-prefix.

With -z nokeep-text-section-prefix, an input section .text.hot.foo will go to the output section .text, instead of .text.hot. Why do you need the option?

MaskRay added inline comments.Sep 26 2019, 5:52 AM
clang/include/clang/Basic/CodeGenOptions.h
113

Comment its allowed values ("all", "labels", "none")

clang/lib/CodeGen/CodeGenModule.cpp
1103 ↗(On Diff #221825)
if (getCodeGenOpts().UniqueInternalFuncNames &&
    isa<FunctionDecl>(GD.getDecl()) &&
    getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {

How does it interop with MangledName = getCUDARuntime().getDeviceStubName(MangledName); below?

clang/lib/Driver/ToolChains/Clang.cpp
4862

The comment is a bit verbose. I think it can just state that when -fpropeller-optimize= or -fpropeller-label is specified, default to -funique-internal-funcnames, followed by a reason why -funique-internal-funcnames should be used.

clang/test/CodeGen/unique_internal_funcnames.c
18 ↗(On Diff #221825)

foo.${{[0-9a-f]+}} (to make it clearer)

tmsriram updated this revision to Diff 222035.Sep 26 2019, 3:04 PM

Updated patch to address the comments.

tmsriram marked 13 inline comments as done.Sep 26 2019, 3:12 PM
tmsriram added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
367

I am unable to find a doc for this. I added comments on this based on what "shouldRelocateWithSymbol" in ELFObjectWriter does and why it prefers relocating with sections rather than symbols.

clang/include/clang/Driver/Options.td
1974

Yes, -fpropeller-label is the umbrella propeller option that turns on -fbasicblock-sections=labels.

clang/lib/CodeGen/CGDeclCXX.cpp
348 ↗(On Diff #221825)

Yes, this is related to function sections getting it right which is important for Propeller.

clang/lib/CodeGen/CodeGenModule.cpp
1032 ↗(On Diff #221825)

Removed it and will add it independently.

1103 ↗(On Diff #221825)

The .stub will be suffixed and it will demangle correctly.

clang/lib/Driver/ToolChains/Gnu.cpp
631 ↗(On Diff #221825)

I see what you mean, can we follow this up in some manner with a check to see if the underlying linker supports Propeller?

640 ↗(On Diff #221825)

Call graph profile sort conflicts with the reordering we do and cannot be on at the same time. We have plans to merge the two, so until then.

642 ↗(On Diff #221825)

We are planning to restore keep-text-section-prefix in some manner with Propeller. Since propeller shuffles sections what is hot is not clearly defined by a prefix so this option won't make sense with Propeller. We will use a heuristic to compute hotness and then regenerate the section markers in the final binary.

MaskRay added inline comments.Sep 26 2019, 8:21 PM
clang/lib/Driver/ToolChains/Gnu.cpp
631 ↗(On Diff #221825)

Some systems install lld at /usr/bin/ld. This will work even if -fuse-ld=lld is not specified. lld can also be used with -fuse-ld=/path/to/ld.lld . I think the best is just not to have the check.

642 ↗(On Diff #221825)

OK, thanks for the clarification. The two disabled features deserve comments, even if they are TODO.

tmsriram updated this revision to Diff 238552.Jan 16 2020, 11:27 AM

Updated to top of trunk, bug fixes.

tmsriram updated this revision to Diff 238620.Jan 16 2020, 2:17 PM

clang-formatted.

tmsriram updated this revision to Diff 242196.Feb 3 2020, 2:41 PM

Splitting the clang patch into several pieces:

  1. This is the parent patch and just contains support for basic block section options.
  2. A separate patch for unique internal function names
  3. A separate patch for an option to relocate with symbols instead of sections
  4. A separate patch for Propeller flags for clang

If you don't mind, I can push a Diff to this Differential which will address these review comments.

clang/lib/CodeGen/BackendUtil.cpp
446

MemoryBuffer::getFile

avoid fstream.

clang/lib/Frontend/CompilerInvocation.cpp
958

This patch requires a rebase.

Opts.UniqueBBSectionNames = Args.hasArg(OPT_funique_bb_section_names);

-ffoo and -fno-foo should have a test/Driver/ test.

There is no need to test both -ffoo and -fno-foo for cc1 options.

grimar added a subscriber: grimar.Feb 10 2020, 1:53 AM
grimar added inline comments.
clang/include/clang/Basic/CodeGenOptions.h
113

I'd suggest to rewrite it somehow. This set of values did not help me to understand what is this field for. The comment could probably be (for example): "This is a field for.... Allowed values are:...."

clang/lib/CodeGen/BackendUtil.cpp
454

Something is wrong with the namings (I am not an expert in lib/CodeGen), but you are mixing lower vs upper case styles: "fin", "line", "S", "R". Seems the code around prefers upper case.

460
if (S.empty() || S[0] == '@' || S[0] == '#')
  continue;
if (!S.consume_front("!") || S.empty())
  break;
if (S.consume_front("!")) {

It is looks a bit strange. See: you are testing S.empty() condition twice and you are checking S.consume_front("!") twice.

471

It seems this assert can be triggered for a wrong user input?

clang/lib/Frontend/CompilerInvocation.cpp
962

Doesn't seem you need "{}" around this lines. (Its a single call and looks inconsistent with the code around).
(The same for the change above)

If you don't mind, I can push a Diff to this Differential which will address these review comments.

Let me update this patch asap as we refactored getBBSectionsList into llvm as it is shared by llc, clang and lld.

tmsriram updated this revision to Diff 243614.Feb 10 2020, 10:11 AM
tmsriram marked 3 inline comments as done.

Removed getBBSectionsList (moved to LLVM) and address other reviewer comments.

MaskRay added a comment.EditedFeb 10 2020, 9:07 PM

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 'propeller' in namespace 'llvm'
    llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,

Chatted with shenhan and xur offline. I tend to agree that -fbasicblock-sections=label can improve profile accuracy. It'd be nice to make that feature separate,
even if there is still a debate on whether the rest of Propeller is done in a maintainable way.

I think the patch series should probably be structured this way:

  1. LLVM CodeGen: enables basic block sections.
  2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
  1. LLVM CodeGen: which enables the rest of Propeller options.
  2. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of Propeller features. It should not do hacky diassembly tasks.
  3. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4

Making 1 and 2 separate can help move forward the patch series. 1 and 2 should not reference llvm::propeller.

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

Weird, getBBSectionsList is defined by D68063. Let me try doing that again. I will also address the rest of your comments.

clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 'propeller' in namespace 'llvm'
    llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,

Chatted with shenhan and xur offline. I tend to agree that -fbasicblock-sections=label can improve profile accuracy. It'd be nice to make that feature separate,
even if there is still a debate on whether the rest of Propeller is done in a maintainable way.

I think the patch series should probably be structured this way:

  1. LLVM CodeGen: enables basic block sections.
  2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
  3. LLVM CodeGen: which enables the rest of Propeller options.
  4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of Propeller features. It should not do hacky diassembly tasks.
  5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4

    Making 1 and 2 separate can help move forward the patch series. 1 and 2 should not reference llvm::propeller.
clang/lib/CodeGen/BackendUtil.cpp
454

I am moving this function out of clang into llvm as this needs to be shared by llc, llvm and lld. I will address all your comments for this function in the llvm change.

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

Darn, the include of "llvm/ProfileData/BBSectionsProf.h" is missing in BackendUtil.cpp. Made this mistake when splitting the patch. I will fix this shortly.

Weird, getBBSectionsList is defined by D68063. Let me try doing that again. I will also address the rest of your comments.

clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 'propeller' in namespace 'llvm'
    llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,

Chatted with shenhan and xur offline. I tend to agree that -fbasicblock-sections=label can improve profile accuracy. It'd be nice to make that feature separate,
even if there is still a debate on whether the rest of Propeller is done in a maintainable way.

I think the patch series should probably be structured this way:

  1. LLVM CodeGen: enables basic block sections.
  2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
  3. LLVM CodeGen: which enables the rest of Propeller options.
  4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of Propeller features. It should not do hacky diassembly tasks.
  5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4

    Making 1 and 2 separate can help move forward the patch series. 1 and 2 should not reference llvm::propeller.
tmsriram updated this revision to Diff 243947.Feb 11 2020, 12:08 PM

Remove usage of "propeller". Fix header inclusion.

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

This should work now. Please apply D68063 first and then this one. Thanks!

Weird, getBBSectionsList is defined by D68063. Let me try doing that again. I will also address the rest of your comments.

clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named 'propeller' in namespace 'llvm'
    llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections,

Chatted with shenhan and xur offline. I tend to agree that -fbasicblock-sections=label can improve profile accuracy. It'd be nice to make that feature separate,
even if there is still a debate on whether the rest of Propeller is done in a maintainable way.

I think the patch series should probably be structured this way:

  1. LLVM CodeGen: enables basic block sections.
  2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
  3. LLVM CodeGen: which enables the rest of Propeller options.
  4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of Propeller features. It should not do hacky diassembly tasks.
  5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4

    Making 1 and 2 separate can help move forward the patch series. 1 and 2 should not reference llvm::propeller.

I think the patch series should probably be structured this way:

  1. LLVM CodeGen: enables basic block sections.
  2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM.
  3. LLVM CodeGen: which enables the rest of Propeller options.
  4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of Propeller features. It should not do hacky diassembly tasks.
  5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4

    Making 1 and 2 separate can help move forward the patch series. 1 and 2 should not reference llvm::propeller.

It is now structured like how you mentioned above. 1) corresponds to D68063 and D73674. 2) corresponds to this patch, D68049. These patches do not reference "propeller" anywhere and are only about basic block sections and labels.

We will address Eli's comments in the LLVM patches. We will also make 3), 4) and 5) like how you wanted. Further, D68065 is about LLD support for basic block sections and will not reference "propeller".

In D68049#1865967, @MaskRay wrote:
If you don't mind, I can push a Diff to this Differential which will address these review comments.

I can't because I can't figure out the patch relationship...

First, this patch does not build on its own. I try applying D68063 first, then this patch. It still does not compile..

This should work now. Please apply D68063 first and then this one. Thanks!

Please can you specify all those patch relations by marking patches as parent/child in phab ui?

tmsriram updated this revision to Diff 253271.Fri, Mar 27, 6:21 PM
tmsriram marked 5 inline comments as done.
tmsriram added a reviewer: eli.friedman.

Clang options for Basic Block Sections enabled in D68063 and D73674

  1. -fbasicblock-sections = { "all" | "<filename>" | "labels" | "none" }
  2. -funique-bb-section-names

Added more tests. Rebased and checked that it applies cleanly on trunk.

tmsriram edited the summary of this revision. (Show Details)Tue, Mar 31, 4:59 PM
MaskRay added inline comments.Wed, Apr 1, 10:58 PM
clang/lib/Driver/ToolChains/Clang.cpp
4858

If we want to pass the option verbatim, A->render(Args, CmdArgs);

However, we actually want (a convention) to catch the error at the driver level. So the value checking in Frontend/CompilerInvocation.cpp should be moved here.

(Note that you used err_drv_invalid_value in Frontend/CompilerInvocation.cpp . drv is short for driver)

clang/test/CodeGen/basicblock-sections.c
36

I haven't read through the previous threads whether we should use a .c -> .s test here. Assume we've decided to do that, @progbits should be followed by {{$}} to ensure there is no unique assembly linkage.

39

If world is followed by a colon, the colon should be added. The little detail will make it clear that this is a label.

tmsriram updated this revision to Diff 254999.Fri, Apr 3, 8:47 PM
tmsriram marked 4 inline comments as done.

Fix test and make error check at driver.

clang/test/CodeGen/basicblock-sections.c
36

Fixed. About .c->.s, this is similar to what function-sections.c does too. Please let me know if I missed anything.