This is an archive of the discontinued LLVM Phabricator instance.

Propeller: Clang options for basic block sections
ClosedPublic

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.

grimar added inline comments.Feb 10 2020, 1:53 AM
clang/lib/CodeGen/BackendUtil.cpp
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.
  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.

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.
  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.

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.
  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.

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.

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.Mar 27 2020, 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)Mar 31 2020, 4:59 PM
MaskRay added inline comments.Apr 1 2020, 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.Apr 3 2020, 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.

Ping.

@rsmith ^^^

More specific question, do you think clang/test/CodeGen/basicblock-sections.c should be converted to a -S -emit-llvm test?

Ping.

@rsmith ^^^

More specific question, do you think clang/test/CodeGen/basicblock-sections.c should be converted to a -S -emit-llvm test?

My understanding is that because this patch only changes TargetOptions, it has no effect on the emitted LLVM IR - only on how that IR is translated to machine code. (like -ffunction-sections, for instance - that property is not persisted in the LLVM IR, it's an API-level communication between Clang and the LLVM backends) So either it goes untested, or it gets tested by a source->assembly test.

Ping.

@rsmith ^^^

More specific question, do you think clang/test/CodeGen/basicblock-sections.c should be converted to a -S -emit-llvm test?

My understanding is that because this patch only changes TargetOptions, it has no effect on the emitted LLVM IR - only on how that IR is translated to machine code. (like -ffunction-sections, for instance - that property is not persisted in the LLVM IR, it's an API-level communication between Clang and the LLVM backends) So either it goes untested, or it gets tested by a source->assembly test.

Yes, David is right. Machine IR after BBSectionsPrepare is the first point at which this option has an effect. This is similar to function-sections.c. I acknowledge the concern that tests should not do more than what is required.

Please add documentation for the new flags.

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

I would prefer to spell this basic-block rather than basicblock, but I don't feel strongly about it.

1975

It's not great to use the same argument as both one of three specific strings and as an arbitrary filename. This not only prevents using those three names as the file name, it also means adding any more specific strings is a backwards-incompatible change. Can you find some way to tweak this to avoid that problem?

1990–1994

I don't like the inconsistency of using bb here and basicblock / basic-block above. Would spelling this out fully (-funique-basic-block-section-names) be OK?

clang/lib/CodeGen/BackendUtil.cpp
486–501

I don't like doing the parsing here. But... this is consistent with what the other options above do, so I'm OK with keep it like this for consistency. (If someone wants to clean this up we can do that separately for all these enum options.)

tmsriram marked an inline comment as done.Apr 21 2020, 10:50 AM
tmsriram added inline comments.
clang/include/clang/Driver/Options.td
1975

Understood, I have the following suggestions:

  1. Would this be ugly? -fbasicblock-sections=list=<filename>
  2. I could make this a completely new option.

Is there a preference? Thanks.

tmsriram updated this revision to Diff 259626.Apr 23 2020, 10:38 AM
tmsriram marked 5 inline comments as done.

Document the flags, rename the options.

tmsriram added inline comments.Apr 23 2020, 10:39 AM
clang/lib/CodeGen/BackendUtil.cpp
486–501

Acknowledged.

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 2:24 PM
shenhan added inline comments.May 5 2020, 4:49 PM
lld/ELF/LTO.cpp
80 ↗(On Diff #259626)

Maybe "--lto-basicblock-sections" should be changed to "--lto-basic-block-sections" too?

tmsriram marked an inline comment as done.May 11 2020, 5:53 PM
tmsriram added inline comments.
lld/ELF/LTO.cpp
80 ↗(On Diff #259626)

Ok if I do this as a separate patch? llc options need to renamed too so I clean up independently after this.

rsmith marked an inline comment as done.May 11 2020, 6:43 PM
rsmith added inline comments.
clang/include/clang/Basic/CodeGenOptions.h
114–127

This comment appears to be out of date with respect to the list= change.

clang/include/clang/Driver/Options.td
1975

Using list= filename seems fine to me.

1976

Missing list= from this description.

Please also specify Values<"all,labels,none,list="> here so that we can tab-complete these values.

clang/lib/CodeGen/BackendUtil.cpp
498–499

Please emit a proper diagnostic for this rather than writing to stderr directly. We should be able to pass the DiagnosticsEngine into here.

clang/test/CodeGen/basic-block-sections.funcnames
1 ↗(On Diff #263322)

This file should be moved to the Inputs subdirectory (top-level files in this directory should generally be tests).

tmsriram updated this revision to Diff 263371.May 12 2020, 12:13 AM
tmsriram marked 5 inline comments as done and an inline comment as not done.

Address reviewer comments:

  1. Use Diagnostic instead of errs
  2. Move test input to Inputs/
  3. Fix option description.

Ping.

clang/include/clang/Basic/CodeGenOptions.h
114–127

Fixed, sorry.

Clang side LG with some minor changes.

clang/docs/ClangCommandLineReference.rst
1336 ↗(On Diff #263371)

This file is automatically generated from the .td file; this text will be lost when the file is auto-generated. To include a custom description here, it should be specified as a DocBrief annotation on the option. (Otherwise we default to using the HelpText.)

clang/docs/UsersManual.rst
1704 ↗(On Diff #263371)

Please either explain here or link to an explanation elsewhere of what "arg" is and how to format it.

clang/lib/CodeGen/BackendUtil.cpp
498–499

Can you register a regular diagnostic message for this (it looks like we register CodeGen diagnostics in include/clang/Basic/DiagnosticFrontendKinds.td) rather than emitting a custom diagnostic?

tmsriram updated this revision to Diff 264733.May 18 2020, 3:17 PM
tmsriram marked 4 inline comments as done.

Address reviewer comments:

  • New diagnostic kind
  • Document list=<arg>
  • Add docbrief
clang/docs/ClangCommandLineReference.rst
1336 ↗(On Diff #263371)

Thanks, I included it in both places like other options seem to be doing.

MaskRay added inline comments.Jun 1 2020, 11:35 AM
lld/ELF/LTO.cpp
79 ↗(On Diff #264733)

--lto-basic-block-sections

I think it should be fine updating it here, and probably preferable, since you already touched some related LLD code. It does not affect any tests. (You did not add any tests for --lto-basicblock-sections in the original LLD patch.)

LLD side changes look good. Are you waiting on an explicit approval from @rmisth ?

Clang side of these changes LGTM

tmsriram marked an inline comment as done.Jun 1 2020, 11:56 AM

LLD side changes look good. Are you waiting on an explicit approval from @rmisth ?

Yes.

lld/ELF/LTO.cpp
79 ↗(On Diff #264733)

Yes, I intend to clean this up and the llc option in a separate cleanup patch so that this doesn't get too complicated. Is that alright?

MaskRay accepted this revision.Jun 1 2020, 12:41 PM
MaskRay added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
962

Please mark this comment as done.

lld/ELF/LTO.cpp
79 ↗(On Diff #264733)

It is ok.

This revision is now accepted and ready to land.Jun 1 2020, 12:41 PM
This revision was automatically updated to reflect the committed changes.
tmsriram marked 2 inline comments as done.