Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
grimar added inline comments.Feb 10 2020, 1:53 AM
clang/lib/CodeGen/BackendUtil.cpp
466
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.

477

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

clang/lib/Frontend/CompilerInvocation.cpp
945

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
460

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
4832

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
35 ↗(On Diff #253271)

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.

38 ↗(On Diff #253271)

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
35 ↗(On Diff #253271)

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
1991

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

1992

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?

2010–2014

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
493–508

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
1992

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
493–508

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
79

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
79

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
1992

Using list= filename seems fine to me.

1993

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
505–506

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

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

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

clang/lib/CodeGen/BackendUtil.cpp
505–506

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

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

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

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
945

Please mark this comment as done.

lld/ELF/LTO.cpp
79

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.