Page MenuHomePhabricator

Propeller: Clang options for basic block sections
Needs ReviewPublic

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

Details

Reviewers
rnk
MaskRay
Summary

Options for basic block sections, unique internal linkage function names.

This is part of the Propeller framework to do post link code layout optimizations. Please see the RFC here: https://groups.google.com/forum/#!msg/llvm-dev/ef3mKzAdJ7U/1shV64BYBAAJ and the detailed RFC doc here: https://github.com/google/llvm-propeller/blob/plo-dev/Propeller_RFC.pdf

This is one in the series of patches for Propeller.

This patch adds the following options to clang:

  1. -funique-internal-funcnames : Makes function names with internal linkage unique (best effort).
  2. -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.
  3. -funique-bb-section-names: Enables unique section names for basic block sections, disabled by default.
  4. -mrelocate-with-symbols: Use symbols for relocations instead of sections.
  5. -fpropeller-label: Enables all options related to Propeller for labelling basic blocks like basic block symbols.
  6. -fpropeller-optimize=<file>: Enables all options related to Propeller for optimizing like basic block sections and associated linker options.

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
52

Nit: indentation is off here.

341

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

clang/include/clang/Driver/Options.td
1873

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

clang/lib/CodeGen/CGDeclCXX.cpp
348

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

clang/lib/CodeGen/CodeGenModule.cpp
1031–1032

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)

1099

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
439
if (S.consume_front("!")) {
  if (S.empty())
    ...
  else
    ... 
}
clang/lib/Driver/ToolChains/Gnu.cpp
631

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

640

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

642

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
120

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

clang/lib/CodeGen/CodeGenModule.cpp
1102
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
4314

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
19

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
341

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
1873

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

clang/lib/CodeGen/CGDeclCXX.cpp
348

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

clang/lib/CodeGen/CodeGenModule.cpp
1031–1032

Removed it and will add it independently.

1102

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

clang/lib/Driver/ToolChains/Gnu.cpp
631

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

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

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

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

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