[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
tmsriram marked 2 inline comments as done.
Closed by commit rGe0bca46b0854: Options for Basic Block Sections, enabled in 
D68063 and D73674. (authored by tmsriram).

Changed prior to commit:
  https://reviews.llvm.org/D68049?vs=264733=267811#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/Inputs/basic-block-sections.funcnames
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
===
--- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -879,7 +879,7 @@
 Name += MBB.getParent()->getName();
   } else {
 Name += MBB.getParent()->getSection()->getName();
-if (TM.getUniqueBBSectionNames()) {
+if (TM.getUniqueBasicBlockSectionNames()) {
   Name += ".";
   Name += MBB.getSymbol()->getName();
 } else {
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep ".strtab" sizes
 /// small.
 void MachineFunction::createBBLabels() {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -78,7 +78,7 @@
 CGOPT(unsigned, TLSSize)
 CGOPT(bool, EmulatedTLS)
 CGOPT(bool, UniqueSectionNames)
-CGOPT(bool, UniqueBBSectionNames)
+CGOPT(bool, UniqueBasicBlockSectionNames)
 CGOPT(EABI, EABIVersion)
 CGOPT(DebuggerKind, DebuggerTuningOpt)
 CGOPT(bool, EnableStackSizeSection)
@@ -350,11 +350,11 @@
   cl::init(true));
   CGBINDOPT(UniqueSectionNames);
 
-  static cl::opt UniqueBBSectionNames(
+  static cl::opt UniqueBasicBlockSectionNames(
   "unique-bb-section-names",
   cl::desc("Give unique names to every basic block section"),
   cl::init(false));
-  CGBINDOPT(UniqueBBSectionNames);
+  CGBINDOPT(UniqueBasicBlockSectionNames);
 
   static cl::opt EABIVersion(
   "meabi", cl::desc("Set EABI type (default depends on triple):"),
@@ -460,7 +460,7 @@
   Options.FunctionSections = getFunctionSections();
   Options.BBSections = getBBSectionsMode(Options);
   Options.UniqueSectionNames = getUniqueSectionNames();
-  Options.UniqueBBSectionNames = getUniqueBBSectionNames();
+  Options.UniqueBasicBlockSectionNames = getUniqueBasicBlockSectionNames();
   Options.TLSSize = getTLSSize();
   Options.EmulatedTLS = getEmulatedTLS();
   Options.ExplicitEmulatedTLS = EmulatedTLSView->getNumOccurrences() > 0;
Index: llvm/lib/CodeGen/BBSectionsPrepare.cpp
===
--- llvm/lib/CodeGen/BBSectionsPrepare.cpp
+++ llvm/lib/CodeGen/BBSectionsPrepare.cpp
@@ -9,15 +9,15 @@
 // BBSectionsPrepare implementation.
 //
 // The purpose of this pass is to assign sections to basic blocks when
-// -fbasicblock-sections= option is used. Further, with profile information only
-// the subset of basic blocks with profiles are placed in separate sections and
-// the rest are grouped in a cold section. The exception handling blocks are
+// -fbasic-block-sections= option is used. Further, with profile information
+// only the subset of basic blocks with profiles are placed in separate sections
+// and the rest are grouped in a cold section. The exception handling blocks are
 // treated specially to ensure they are all in one seciton.
 //
 // Basic Block Sections
 // 
 //
-// With option, -fbasicblock-sections=list, every function may be split into
+// With option, -fbasic-block-sections=list, every 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:964
+<< Opts.BBSections;
+  }
+

grimar wrote:
> 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)
Please mark this comment as done.



Comment at: lld/ELF/LTO.cpp:79
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

tmsriram wrote:
> MaskRay wrote:
> > `--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.)
> 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?
It is ok.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added a comment.

In D68049#2066937 , @MaskRay wrote:

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


Yes.




Comment at: lld/ELF/LTO.cpp:79
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

MaskRay wrote:
> `--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.)
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Clang side of these changes LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: lld/ELF/LTO.cpp:79
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:1336
+
+Generate labels for each basic block or place each basic block or a subset of 
basic blocks in its own section
+

rsmith wrote:
> 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`.)
Thanks, I included it in both places like other options seem to be doing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 264733.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Address reviewer comments:

- New diagnostic kind
- Document list=
- Add docbrief


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/Inputs/basic-block-sections.funcnames
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1698,6 +1698,44 @@
  $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
  $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  With the ``list=`` option, a file containing the subset of basic blocks
+  that need to placed in unique sections can be specified.  The format of the
+  file is as follows.  For example, ``list=spec.txt`` where ``spec.txt`` is the
+  following:
+
+  ::
+
+!foo
+!!2
+!_Z3barv
+
+  will place the machine basic block with ``id 2`` in function ``foo`` in a
+  unique section.  It will also place all basic blocks of functions ``bar``
+  in unique sections.
+
+  Further, section clusters can also be specified using the ``list=``
+  option.  For example, ``list=spec.txt`` where ``spec.txt`` contains:
+
+  ::
+
+!foo
+!!1 !!3 !!5
+!!2 !!4 !!6
+
+  will create two unique sections for function ``foo`` with the first
+  containing the odd numbered basic blocks and the second containing the
+  even numbered basic blocks.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section.
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -929,7 +929,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -169,7 +169,7 @@
   bool ltoDebugPassManager;
   bool ltoEmitAsm;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -78,7 +78,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
   

[PATCH] D68049: Propeller: Clang options for basic block sections

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Clang side LG with some minor changes.




Comment at: clang/docs/ClangCommandLineReference.rst:1336
+
+Generate labels for each basic block or place each basic block or a subset of 
basic blocks in its own section
+

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



Comment at: clang/docs/UsersManual.rst:1704
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.

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



Comment at: clang/lib/CodeGen/BackendUtil.cpp:503-504
+if (!MBOrErr)
+  errs() << "Error loading basic block sections function list file: "
+ << MBOrErr.getError().message() << "\n";
+else

rsmith wrote:
> Please emit a proper diagnostic for this rather than writing to stderr 
> directly. We should be able to pass the `DiagnosticsEngine` into here.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.




Comment at: clang/include/clang/Basic/CodeGenOptions.h:114-127
+  // -fbasic-block-sections=.  The allowed values with this option are:
+  // {"labels", "all", "", "none"}.
+  //
+  // "labels": Only generate basic block symbols (labels) for all basic
+  //   blocks, do not generate unique sections for basic blocks.
+  //   Use the machine basic block id in the symbol name to
+  //   associate profile info from virtual address to machine

rsmith wrote:
> This comment appears to be out of date with respect to the `list=` change.
Fixed, sorry.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 263371.
tmsriram marked 5 inline comments as done and an inline comment as not done.
tmsriram added a comment.

Address reviewer comments:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/Inputs/basic-block-sections.funcnames
  clang/test/CodeGen/basic-block-sections.c
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1698,6 +1698,16 @@
  $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
  $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -930,7 +930,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -169,7 +169,7 @@
   bool ltoDebugPassManager;
   bool ltoEmitAsm;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -79,7 +79,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
 if (config->ltoBasicBlockSections == "all") {
   c.Options.BBSections = BasicBlockSection::All;
@@ -100,7 +100,8 @@
 }
   }
 
-  c.Options.UniqueBBSectionNames = config->ltoUniqueBBSectionNames;
+  c.Options.UniqueBasicBlockSectionNames =
+  config->ltoUniqueBasicBlockSectionNames;
 
   if (auto relocModel = getRelocModelFromCMModel())
 c.RelocModel = *relocModel;
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked an inline comment as done.
rsmith added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:114-127
+  // -fbasic-block-sections=.  The allowed values with this option are:
+  // {"labels", "all", "", "none"}.
+  //
+  // "labels": Only generate basic block symbols (labels) for all basic
+  //   blocks, do not generate unique sections for basic blocks.
+  //   Use the machine basic block id in the symbol name to
+  //   associate profile info from virtual address to machine

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



Comment at: clang/include/clang/Driver/Options.td:1975
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

tmsriram wrote:
> rsmith wrote:
> > 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?
> Understood, I have the following suggestions:
> 
> 1) Would this be ugly?  -fbasicblock-sections=list=
> 2) I could make this a completely new option.
> 
> Is there a preference?  Thanks.
Using `list=` filename seems fine to me.



Comment at: clang/include/clang/Driver/Options.td:1993
+  Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

Missing `list=` from this description.

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



Comment at: clang/lib/CodeGen/BackendUtil.cpp:503-504
+if (!MBOrErr)
+  errs() << "Error loading basic block sections function list file: "
+ << MBOrErr.getError().message() << "\n";
+else

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



Comment at: clang/test/CodeGen/basic-block-sections.funcnames:1
+!world

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: lld/ELF/LTO.cpp:80
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

shenhan wrote:
> Maybe "--lto-basicblock-sections" should be changed to 
> "--lto-basic-block-sections" too?
Ok if I do this as a separate patch?  llc options need to renamed too so I 
clean up independently after this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 263322.
tmsriram added a comment.

Rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basic-block-sections.c
  clang/test/CodeGen/basic-block-sections.funcnames
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1698,6 +1698,16 @@
  $ cd $P/bar && clang -c -funique-internal-linkage-names name_conflict.c
  $ cd $P && clang foo/name_conflict.o && bar/name_conflict.o
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -930,7 +930,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -169,7 +169,7 @@
   bool ltoDebugPassManager;
   bool ltoEmitAsm;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -79,7 +79,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
 if (config->ltoBasicBlockSections == "all") {
   c.Options.BBSections = BasicBlockSection::All;
@@ -100,7 +100,8 @@
 }
   }
 
-  c.Options.UniqueBBSectionNames = config->ltoUniqueBBSectionNames;
+  c.Options.UniqueBasicBlockSectionNames =
+  config->ltoUniqueBasicBlockSectionNames;
 
   if (auto relocModel = getRelocModelFromCMModel())
 c.RelocModel = *relocModel;
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep ".strtab" sizes
 /// small.
 void MachineFunction::createBBLabels() {
Index: llvm/lib/CodeGen/BBSectionsPrepare.cpp
===
--- 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Han Shen via Phabricator via cfe-commits]

shenhan added inline comments.



Comment at: lld/ELF/LTO.cpp:80
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

@rsmith Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:486-501
+  Options.BBSections =
+  llvm::StringSwitch(CodeGenOpts.BBSections)
+  .Case("all", llvm::BasicBlockSection::All)
+  .Case("labels", llvm::BasicBlockSection::Labels)
+  .Case("none", llvm::BasicBlockSection::None)
+  .Default(llvm::BasicBlockSection::List);
+

rsmith wrote:
> 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.)
Acknowledged.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 259626.
tmsriram marked 5 inline comments as done.
tmsriram added a comment.
Herald added subscribers: dexonsmith, steven_wu, hiraditya, arichardson, emaste.
Herald added a reviewer: espindola.

Document the flags, rename the options.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basic-block-sections.c
  clang/test/CodeGen/basic-block-sections.funcnames
  clang/test/Driver/fbasic-block-sections.c
  clang/test/Driver/funique-basic-block-section-names.c
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/BBSectionsPrepare.cpp
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1677,6 +1677,16 @@
on ELF targets when using the integrated assembler. This flag currently
only has an effect on ELF targets.
 
+**-fbasic-block-sections=[labels, all, list=, none]**
+
+  Controls whether Clang emits a label for each basic block.  Further, with
+  values "all" and "list=arg", each basic block or a subset of basic blocks
+  can be placed in its own unique section.
+
+  Basic block sections allow the linker to reorder basic blocks and enables
+  link-time optimizations like whole program inter-procedural basic block
+  reordering.
+
 Profile Guided Optimization
 ---
 
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1331,6 +1331,10 @@
 
 .. option:: -fautolink, -fno-autolink
 
+.. option:: -fbasic-block-sections=labels, -fbasic-block-sections=all, -fbasic-block-sections=list=, -fbasic-block-sections=none
+
+Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section
+
 .. option:: -fblocks, -fno-blocks
 
 Enable the 'blocks' language feature
Index: lld/ELF/Driver.cpp
===
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -930,7 +930,7 @@
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
   args.getLastArgValue(OPT_lto_basicblock_sections);
-  config->ltoUniqueBBSectionNames =
+  config->ltoUniqueBasicBlockSectionNames =
   args.hasFlag(OPT_lto_unique_bb_section_names,
OPT_no_lto_unique_bb_section_names, false);
   config->mapFile = args.getLastArgValue(OPT_Map);
Index: lld/ELF/Config.h
===
--- lld/ELF/Config.h
+++ lld/ELF/Config.h
@@ -167,7 +167,7 @@
   bool ltoCSProfileGenerate;
   bool ltoDebugPassManager;
   bool ltoNewPassManager;
-  bool ltoUniqueBBSectionNames;
+  bool ltoUniqueBasicBlockSectionNames;
   bool ltoWholeProgramVisibility;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
Index: lld/ELF/LTO.cpp
===
--- lld/ELF/LTO.cpp
+++ lld/ELF/LTO.cpp
@@ -79,7 +79,7 @@
   // Check if basic block sections must be used.
   // Allowed values for --lto-basicblock-sections are "all", "labels",
   // "", or none.  This is the equivalent
-  // of -fbasicblock-sections= flag in clang.
+  // of -fbasic-block-sections= flag in clang.
   if (!config->ltoBasicBlockSections.empty()) {
 if (config->ltoBasicBlockSections == "all") {
   c.Options.BBSections = BasicBlockSection::All;
@@ -100,7 +100,8 @@
 }
   }
 
-  c.Options.UniqueBBSectionNames = config->ltoUniqueBBSectionNames;
+  c.Options.UniqueBasicBlockSectionNames =
+  config->ltoUniqueBasicBlockSectionNames;
 
   if (auto relocModel = getRelocModelFromCMModel())
 c.RelocModel = *relocModel;
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -340,7 +340,7 @@
   MBBNumbering.resize(BlockNo);
 }
 
-/// This is used with -fbasicblock-sections or -fbasicblock-labels option.
+/// This is used with -fbasic-block-sections or -fbasicblock-labels option.
 /// A unary encoding of basic block labels is done to keep ".strtab" sizes
 /// small.
 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked an inline comment as done.
tmsriram added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1975
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

rsmith wrote:
> 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?
Understood, I have the following suggestions:

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

Is there a preference?  Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Please add documentation for the new flags.




Comment at: clang/include/clang/Driver/Options.td:1974
 def fno_function_sections : Flag<["-"], "fno-function-sections">, 
Group;
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;

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



Comment at: clang/include/clang/Driver/Options.td:1975
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

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?



Comment at: clang/include/clang/Driver/Options.td:1990-1994
+def funique_bb_section_names : Flag <["-"], "funique-bb-section-names">,
+  Group, Flags<[CC1Option]>,
+  HelpText<"Use unique names for basic block sections (ELF Only)">;
+def fno_unique_bb_section_names : Flag <["-"], "fno-unique-bb-section-names">,
+  Group;

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?



Comment at: clang/lib/CodeGen/BackendUtil.cpp:486-501
+  Options.BBSections =
+  llvm::StringSwitch(CodeGenOpts.BBSections)
+  .Case("all", llvm::BasicBlockSection::All)
+  .Case("labels", llvm::BasicBlockSection::Labels)
+  .Case("none", llvm::BasicBlockSection::None)
+  .Default(llvm::BasicBlockSection::List);
+

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1972297 , @dblaikie wrote:

> In D68049#1971276 , @MaskRay wrote:
>
> > In D68049#1970825 , @tmsriram 
> > wrote:
> >
> > > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D68049#1971276 , @MaskRay wrote:

> In D68049#1970825 , @tmsriram wrote:
>
> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D68049#1970825 , @tmsriram wrote:

> Ping.


@rsmith ^^^

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.



Comment at: clang/test/CodeGen/basicblock-sections.c:35
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world

MaskRay wrote:
> 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.
Fixed.  About .c->.s, this is similar to what function-sections.c does too. 
Please let me know if I missed anything.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 254999.
tmsriram marked 4 inline comments as done.
tmsriram added a comment.

Fix test and make error check at driver.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/Driver/fbasicblock-sections.c
  clang/test/Driver/funique-bb-section-names.c

Index: clang/test/Driver/fbasicblock-sections.c
===
--- /dev/null
+++ clang/test/Driver/fbasicblock-sections.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -fbasicblock-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -fbasicblock-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -fbasicblock-sections=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -fbasicblock-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+//
+// CHECK-OPT-NONE: "-fbasicblock-sections=none"
+// CHECK-OPT-ALL: "-fbasicblock-sections=all"
+// CHECK-OPT-LIST: -fbasicblock-sections={{[^ ]*}}fbasicblock-sections.c
+// CHECK-OPT-LABELS: "-fbasicblock-sections=labels"
Index: clang/test/Driver/funique-bb-section-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-bb-section-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-bb-section-names -fno-unique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-bb-section-names"
+// CHECK-NOOPT-NOT: "-funique-bb-section-names"
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world:
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world:
+// BB_LABELS: a.BB.world:
+// BB_LABELS: aa.BB.world:
+// BB_LABELS: a.BB.another:
+//
+// BB_WORLD: .section .text.world,"ax",@progbits{{$}}
+// BB_WORLD: world:
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world:
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another:
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another:
+// BB_LIST-NOT: a.BB.another:
+//
+// UNIQUE: .section .text.world.a.BB.world,
+// UNIQUE: .section .text.another.a.BB.another,
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -954,10 +954,18 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  std::string(Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none"));
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasArg(OPT_ffunction_sections) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4858
 
+  if (Arg *A = Args.getLastArg(options::OPT_fbasicblock_sections_EQ)) {
+CmdArgs.push_back(

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)



Comment at: clang/test/CodeGen/basicblock-sections.c:35
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world

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.



Comment at: clang/test/CodeGen/basicblock-sections.c:38
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 253271.
tmsriram marked 5 inline comments as done.
tmsriram added a reviewer: eli.friedman.
tmsriram added a comment.

Clang options for Basic Block Sections enabled in D68063 
 and D73674 

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

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/Driver/fbasicblock-sections.c
  clang/test/Driver/funique-bb-section-names.c

Index: clang/test/Driver/fbasicblock-sections.c
===
--- /dev/null
+++ clang/test/Driver/fbasicblock-sections.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -fbasicblock-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -fbasicblock-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -fbasicblock-sections=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -fbasicblock-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+//
+// CHECK-OPT-NONE: "-fbasicblock-sections=none"
+// CHECK-OPT-ALL: "-fbasicblock-sections=all"
+// CHECK-OPT-LIST: -fbasicblock-sections={{[^ ]*}}fbasicblock-sections.c
+// CHECK-OPT-LABELS: "-fbasicblock-sections=labels"
Index: clang/test/Driver/funique-bb-section-names.c
===
--- /dev/null
+++ clang/test/Driver/funique-bb-section-names.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -funique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -funique-bb-section-names -fno-unique-bb-section-names %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-funique-bb-section-names"
+// CHECK-NOOPT-NOT: "-funique-bb-section-names"
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -953,11 +953,24 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  

[PATCH] D68049: Propeller: Clang options for basic block sections

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D68049#1870401 , @tmsriram wrote:

> In D68049#1870094 , @tmsriram wrote:
>
> > In D68049#1868623 , @MaskRay wrote:
> >
> > > > 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1870094 , @tmsriram wrote:

> In D68049#1868623 , @MaskRay wrote:
>
> > > 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`.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 243947.
tmsriram added a comment.

Remove usage of "propeller". Fix header inclusion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c

Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -962,10 +962,24 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  std::string(Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none"));
+  if (Opts.BBSections != "all" && Opts.BBSections != "labels" &&
+  Opts.BBSections != "none" && !llvm::sys::fs::exists(Opts.BBSections)) {
+Diags.Report(diag::err_drv_invalid_value)
+<< Args.getLastArg(OPT_fbasicblock_sections_EQ)->getAsString(Args)
+<< Opts.BBSections;
+  }
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasArg(OPT_ffunction_sections) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueBBSectionNames = Args.hasArg(OPT_funique_bb_section_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4202,8 +4202,11 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_fbasicblock_sections_EQ,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
+options::OPT_funique_bb_section_names,
+options::OPT_fno_unique_bb_section_names,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4758,6 +4761,12 @@
 CmdArgs.push_back("-ffunction-sections");
   }
 
+
+  if (Arg *A = Args.getLastArg(options::OPT_fbasicblock_sections_EQ)) {
+CmdArgs.push_back(
+Args.MakeArgString(Twine("-fbasicblock-sections=") + A->getValue()));
+  }
+
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
UseSeparateSections)) {
 CmdArgs.push_back("-fdata-sections");
@@ -4767,6 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1870094 , @tmsriram wrote:

> In D68049#1868623 , @MaskRay wrote:
>
> > > 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`.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1868623 , @MaskRay wrote:

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






Comment at: clang/lib/CodeGen/BackendUtil.cpp:444
+  while ((std::getline(fin, line)).good()) {
+StringRef S(line);
+// Lines beginning with @, # are not useful here.

grimar wrote:
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 243614.
tmsriram marked 3 inline comments as done.
tmsriram added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c

Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -962,10 +962,24 @@
   Opts.TrapFuncName = std::string(Args.getLastArgValue(OPT_ftrap_function_EQ));
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasArg(OPT_ffunction_sections);
+  Opts.BBSections =
+  std::string(Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none"));
+  if (Opts.BBSections != "all" && Opts.BBSections != "labels" &&
+  Opts.BBSections != "none" && !llvm::sys::fs::exists(Opts.BBSections)) {
+Diags.Report(diag::err_drv_invalid_value)
+<< Args.getLastArg(OPT_fbasicblock_sections_EQ)->getAsString(Args)
+<< Opts.BBSections;
+  }
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasArg(OPT_ffunction_sections) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
+  Opts.UniqueBBSectionNames = Args.hasArg(OPT_funique_bb_section_names);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4202,8 +4202,11 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_fbasicblock_sections_EQ,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
+options::OPT_funique_bb_section_names,
+options::OPT_fno_unique_bb_section_names,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4758,6 +4761,11 @@
 CmdArgs.push_back("-ffunction-sections");
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_fbasicblock_sections_EQ)) {
+CmdArgs.push_back(
+Args.MakeArgString(Twine("-fbasicblock-sections=") + A->getValue()));
+  }
+
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D68049#1865967 , @MaskRay wrote:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:120
 
+  std::string BasicBlockSections;
+

MaskRay wrote:
> Comment its allowed values ("all", "labels", "none")
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:"



Comment at: clang/lib/CodeGen/BackendUtil.cpp:444
+  while ((std::getline(fin, line)).good()) {
+StringRef S(line);
+// Lines beginning with @, # are not useful here.

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.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:450
+  break;
+if (S.consume_front("!")) {
+  if (fi != Options.BBSectionsList.end())

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



Comment at: clang/lib/CodeGen/BackendUtil.cpp:461
+  fi = R.first;
+  assert(R.second);
+}

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



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:964
+<< Opts.BBSections;
+  }
+

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)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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




Comment at: clang/lib/CodeGen/BackendUtil.cpp:436
+
+  std::ifstream fin(FunctionsListFile);
+  if (!fin.good()) {

`MemoryBuffer::getFile`

avoid fstream.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:977
  OPT_fno_unique_section_names, true);
+  Opts.UniqueBBSectionNames = Args.hasFlag(
+  OPT_funique_bb_section_names, OPT_fno_unique_bb_section_names, false);

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 242196.
tmsriram added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c

Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+int world(int a) {
+  if (a > 10)
+return 10;
+  else if (a > 5)
+return 5;
+  else
+return 0;
+}
+
+int another(int a) {
+  if (a > 10)
+return 20;
+  return 0;
+}
+
+// PLAIN-NOT: section
+// PLAIN: world
+//
+// BB_LABELS-NOT: section
+// BB_LABELS: world
+// BB_LABELS-LABEL: a.BB.world
+// BB_LABELS-LABEL: aa.BB.world
+// BB_LABEL-LABEL: a.BB.another
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
+// BB_ALL: .section .text.another,"ax",@progbits,unique
+// BB_ALL: a.BB.another
+// BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
+// BB_LIST: another
+// BB_LIST-NOT: a.BB.another
+//
+// UNIQUE: .section .text.world.a.BB.world
+// UNIQUE: .section .text.another.a.BB.another
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -955,14 +955,27 @@
   Opts.TrapFuncName = Args.getLastArgValue(OPT_ftrap_function_EQ);
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
-  Opts.FunctionSections = Args.hasFlag(OPT_ffunction_sections,
-   OPT_fno_function_sections, false);
+  Opts.BBSections = Args.getLastArgValue(OPT_fbasicblock_sections_EQ, "none");
+  if (Opts.BBSections != "all" && Opts.BBSections != "labels" &&
+  Opts.BBSections != "none" && !llvm::sys::fs::exists(Opts.BBSections)) {
+Diags.Report(diag::err_drv_invalid_value)
+<< Args.getLastArg(OPT_fbasicblock_sections_EQ)->getAsString(Args)
+<< Opts.BBSections;
+  }
+
+  // Basic Block Sections implies Function Sections.
+  Opts.FunctionSections =
+  Args.hasFlag(OPT_ffunction_sections, OPT_fno_function_sections, false) ||
+  (Opts.BBSections != "none" && Opts.BBSections != "labels");
+
   Opts.DataSections = Args.hasFlag(OPT_fdata_sections,
OPT_fno_data_sections, false);
   Opts.StackSizeSection =
   Args.hasFlag(OPT_fstack_size_section, OPT_fno_stack_size_section, false);
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
  OPT_fno_unique_section_names, true);
+  Opts.UniqueBBSectionNames = Args.hasFlag(
+  OPT_funique_bb_section_names, OPT_fno_unique_bb_section_names, false);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4153,8 +4153,11 @@
 options::OPT_fno_function_sections,
 options::OPT_fdata_sections,
 options::OPT_fno_data_sections,
+options::OPT_fbasicblock_sections_EQ,
 options::OPT_funique_section_names,
 options::OPT_fno_unique_section_names,
+options::OPT_funique_bb_section_names,
+

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 238620.
tmsriram added a comment.

clang-formatted.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/CodeGen/unique_internal_funcnames.c
  clang/test/Driver/propeller-flags.c
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -90,6 +90,7 @@
   unsigned SaveTemporaryLabels : 1;
   unsigned GenDwarfForAssembly : 1;
   unsigned RelaxELFRelocations : 1;
+  unsigned RelocateWithSymbols : 1;
   unsigned DwarfVersion;
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
@@ -236,6 +237,7 @@
   }
 
   Opts.RelaxELFRelocations = Args.hasArg(OPT_mrelax_relocations);
+  Opts.RelocateWithSymbols = Args.hasArg(OPT_mrelocate_with_symbols);
   Opts.DwarfVersion = getLastArgIntValue(Args, OPT_dwarf_version_EQ, 2, Diags);
   Opts.DwarfDebugFlags = Args.getLastArgValue(OPT_dwarf_debug_flags);
   Opts.DwarfDebugProducer = Args.getLastArgValue(OPT_dwarf_debug_producer);
@@ -363,6 +365,7 @@
   MAI->setCompressDebugSections(Opts.CompressDebugSections);
 
   MAI->setRelaxELFRelocations(Opts.RelaxELFRelocations);
+  MAI->setRelocateWithSymbols(Opts.RelocateWithSymbols);
 
   bool IsBinary = Opts.OutputType == AssemblerInvocation::FT_Obj;
   if (Opts.OutputPath.empty())
Index: clang/test/Driver/propeller-flags.c
===
--- /dev/null
+++ clang/test/Driver/propeller-flags.c
@@ -0,0 +1,13 @@
+// Check that -fpropeller flag invokes the correct options.
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-label -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_LABEL
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-optimize=perf.propeller -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_OPT
+
+// CHECK_PROPELLER_LABEL: "-fbasicblock-sections=labels"
+// CHECK_PROPELLER_LABEL: "-funique-internal-funcnames"
+// CHECK_PROPELLER_LABEL: "--lto-basicblock-sections=labels"
+//
+// CHECK_PROPELLER_OPT: "-fbasicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "-funique-internal-funcnames"
+// CHECK_PROPELLER_OPT: "--propeller=perf.propeller"
+// CHECK_PROPELLER_OPT: "--lto-basicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "--optimize-bb-jumps"
Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.$
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o - < %s | FileCheck %s 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 238552.
tmsriram added a comment.

Updated to top of trunk, bug fixes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/CodeGen/unique_internal_funcnames.c
  clang/test/Driver/propeller-flags.c
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -90,6 +90,7 @@
   unsigned SaveTemporaryLabels : 1;
   unsigned GenDwarfForAssembly : 1;
   unsigned RelaxELFRelocations : 1;
+  unsigned RelocateWithSymbols : 1;
   unsigned DwarfVersion;
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
@@ -236,6 +237,7 @@
   }
 
   Opts.RelaxELFRelocations = Args.hasArg(OPT_mrelax_relocations);
+  Opts.RelocateWithSymbols = Args.hasArg(OPT_mrelocate_with_symbols);
   Opts.DwarfVersion = getLastArgIntValue(Args, OPT_dwarf_version_EQ, 2, Diags);
   Opts.DwarfDebugFlags = Args.getLastArgValue(OPT_dwarf_debug_flags);
   Opts.DwarfDebugProducer = Args.getLastArgValue(OPT_dwarf_debug_producer);
@@ -363,6 +365,7 @@
   MAI->setCompressDebugSections(Opts.CompressDebugSections);
 
   MAI->setRelaxELFRelocations(Opts.RelaxELFRelocations);
+  MAI->setRelocateWithSymbols(Opts.RelocateWithSymbols);
 
   bool IsBinary = Opts.OutputType == AssemblerInvocation::FT_Obj;
   if (Opts.OutputPath.empty())
Index: clang/test/Driver/propeller-flags.c
===
--- /dev/null
+++ clang/test/Driver/propeller-flags.c
@@ -0,0 +1,13 @@
+// Check that -fpropeller flag invokes the correct options.
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-label -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_LABEL
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-optimize=perf.propeller -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_OPT
+
+// CHECK_PROPELLER_LABEL: "-fbasicblock-sections=labels"
+// CHECK_PROPELLER_LABEL: "-funique-internal-funcnames"
+// CHECK_PROPELLER_LABEL: "--lto-basicblock-sections=labels"
+//
+// CHECK_PROPELLER_OPT: "-fbasicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "-funique-internal-funcnames"
+// CHECK_PROPELLER_OPT: "--propeller=perf.propeller"
+// CHECK_PROPELLER_OPT: "--lto-basicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "--optimize-bb-jumps"
Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,18 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+
+static int foo() {
+  return 0;
+}
+
+int (*bar()) () {
+   return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.$
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names -o 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:631
+if (A->getOption().matches(options::OPT_fpropeller_optimize_EQ)) {
+  if (!Args.getLastArgValue(options::OPT_fuse_ld_EQ).equals_lower("lld"))
+D.Diag(clang::diag::err_drv_unsupported_opt)

tmsriram wrote:
> MaskRay wrote:
> > This check is overly constrained. Some systems default to use lld (e.g. 
> > installed at /usr/bin/ld). I suggest removing this check.
> I see what you mean, can we follow this up in some manner with a check to see 
> if the underlying linker supports Propeller?  
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.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:642
+  CmdArgs.push_back("-z");
+  CmdArgs.push_back("nokeep-text-section-prefix");
+  CmdArgs.push_back("--no-warn-symbol-ordering");

tmsriram wrote:
> MaskRay wrote:
> > 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?
> 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.  
OK, thanks for the clarification. The two disabled features deserve comments, 
even if they are TODO.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked 13 inline comments as done.
tmsriram added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:341
+CODEGENOPT(RelocateWithSymbols, 1, 0)
+
 /// Whether we should use the undefined behaviour optimization for control flow

mehdi_amini wrote:
> Can you add a doc here? (possibly referring to somewhere else if it is 
> extensively documented elsewhere?)
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.



Comment at: clang/include/clang/Driver/Options.td:1873
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

mehdi_amini wrote:
> Is the "labels" options dependent/related to the previous -fpropeller-label 
> one?
Yes, -fpropeller-label is the umbrella propeller option that turns on 
-fbasicblock-sections=labels.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:348
+Twine NewName = Section + Name;
+Fn->setSection(NewName.str());
+  }

mehdi_amini wrote:
> Is this related to these new option? It this change the behavior of 
> -ffunction-sections?
Yes, this is related to function sections getting it right which is important 
for Propeller.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1032
 }
-
+  }
   return Out.str();

mehdi_amini wrote:
> 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)
Removed it and will add it independently.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1103
+  dyn_cast(GD.getDecl()) &&
+  this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
+std::string UniqueSuffix = getUniqueModuleId((), true);

MaskRay wrote:
> ```
>   if (getCodeGenOpts().UniqueInternalFuncNames &&
>   isa(GD.getDecl()) &&
>   getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
> ```
> 
> How does it interop with `MangledName = 
> getCUDARuntime().getDeviceStubName(MangledName);` below?
The .stub will be suffixed and it will demangle correctly.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:631
+if (A->getOption().matches(options::OPT_fpropeller_optimize_EQ)) {
+  if (!Args.getLastArgValue(options::OPT_fuse_ld_EQ).equals_lower("lld"))
+D.Diag(clang::diag::err_drv_unsupported_opt)

MaskRay wrote:
> This check is overly constrained. Some systems default to use lld (e.g. 
> installed at /usr/bin/ld). I suggest removing this check.
I see what you mean, can we follow this up in some manner with a check to see 
if the underlying linker supports Propeller?  



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:640
+  CmdArgs.push_back("--optimize-bb-jumps");
+  CmdArgs.push_back("--no-call-graph-profile-sort");
+  CmdArgs.push_back("-z");

MaskRay wrote:
> Why --no-call-graph-profile-sort?
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.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:642
+  CmdArgs.push_back("-z");
+  CmdArgs.push_back("nokeep-text-section-prefix");
+  CmdArgs.push_back("--no-warn-symbol-ordering");

MaskRay wrote:
> 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?
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.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram updated this revision to Diff 222035.
tmsriram added a comment.

Updated patch to address the comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/CodeGen/unique_internal_funcnames.c
  clang/test/Driver/propeller-flags.c
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -90,6 +90,7 @@
   unsigned SaveTemporaryLabels : 1;
   unsigned GenDwarfForAssembly : 1;
   unsigned RelaxELFRelocations : 1;
+  unsigned RelocateWithSymbols : 1;
   unsigned DwarfVersion;
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
@@ -236,6 +237,7 @@
   }
 
   Opts.RelaxELFRelocations = Args.hasArg(OPT_mrelax_relocations);
+  Opts.RelocateWithSymbols = Args.hasArg(OPT_mrelocate_with_symbols);
   Opts.DwarfVersion = getLastArgIntValue(Args, OPT_dwarf_version_EQ, 2, Diags);
   Opts.DwarfDebugFlags = Args.getLastArgValue(OPT_dwarf_debug_flags);
   Opts.DwarfDebugProducer = Args.getLastArgValue(OPT_dwarf_debug_producer);
@@ -361,6 +363,7 @@
   MAI->setCompressDebugSections(Opts.CompressDebugSections);
 
   MAI->setRelaxELFRelocations(Opts.RelaxELFRelocations);
+  MAI->setRelocateWithSymbols(Opts.RelocateWithSymbols);
 
   bool IsBinary = Opts.OutputType == AssemblerInvocation::FT_Obj;
   if (Opts.OutputPath.empty())
Index: clang/test/Driver/propeller-flags.c
===
--- /dev/null
+++ clang/test/Driver/propeller-flags.c
@@ -0,0 +1,13 @@
+// Check that -fpropeller flag invokes the correct options.
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-label -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_LABEL
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-optimize=perf.propeller -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_OPT
+
+// CHECK_PROPELLER_LABEL: "-fbasicblock-sections=labels"
+// CHECK_PROPELLER_LABEL: "-funique-internal-funcnames"
+// CHECK_PROPELLER_LABEL: "--lto-basicblock-sections=labels"
+//
+// CHECK_PROPELLER_OPT: "-fbasicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "-funique-internal-funcnames"
+// CHECK_PROPELLER_OPT: "--propeller=perf.propeller"
+// CHECK_PROPELLER_OPT: "--lto-basicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "--optimize-bb-jumps"
Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,18 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+
+static int foo() {
+  return 0;
+}
+
+int (*bar()) () {
+   return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.$
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.funcnames
@@ -0,0 +1 @@
+!world
Index: clang/test/CodeGen/basicblock-sections.c
===
--- /dev/null
+++ clang/test/CodeGen/basicblock-sections.c
@@ -0,0 +1,47 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -fbasicblock-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=labels -o - < %s | FileCheck %s --check-prefix=BB_LABELS
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=%S/basicblock-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -fbasicblock-sections=all -funique-bb-section-names 

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:120
 
+  std::string BasicBlockSections;
+

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



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1103
+  dyn_cast(GD.getDecl()) &&
+  this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
+std::string UniqueSuffix = getUniqueModuleId((), true);

```
  if (getCodeGenOpts().UniqueInternalFuncNames &&
  isa(GD.getDecl()) &&
  getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
```

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



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4304
+   options::OPT_fno_propeller)) {
+/* If we specify -funique-internal-funcnames on the command
+   line, we do not need to push it again.

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.



Comment at: clang/test/CodeGen/unique_internal_funcnames.c:18
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.$

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:439
+// Empty '!' implies no more functions.
+if (S.size() == 1 && S[0] == '!')
+  break;

```
if (S.consume_front("!")) {
  if (S.empty())
...
  else
... 
}
```



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:631
+if (A->getOption().matches(options::OPT_fpropeller_optimize_EQ)) {
+  if (!Args.getLastArgValue(options::OPT_fuse_ld_EQ).equals_lower("lld"))
+D.Diag(clang::diag::err_drv_unsupported_opt)

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



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:640
+  CmdArgs.push_back("--optimize-bb-jumps");
+  CmdArgs.push_back("--no-call-graph-profile-sort");
+  CmdArgs.push_back("-z");

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



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:642
+  CmdArgs.push_back("-z");
+  CmdArgs.push_back("nokeep-text-section-prefix");
+  CmdArgs.push_back("--no-warn-symbol-ordering");

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:52
+   ///< Produce unique section names with
+  ///< basic block sections.
 ENUM_CODEGENOPT(FramePointer, FramePointerKind, 2, FramePointerKind::None) /// 
frame-pointer: all,non-leaf,none

Nit: indentation is off here.



Comment at: clang/include/clang/Basic/CodeGenOptions.def:341
+CODEGENOPT(RelocateWithSymbols, 1, 0)
+
 /// Whether we should use the undefined behaviour optimization for control flow

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



Comment at: clang/include/clang/Driver/Options.td:1873
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, 
Group, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : 
all | labels | none | ">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group,

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



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:348
+Twine NewName = Section + Name;
+Fn->setSection(NewName.str());
+  }

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



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1032
 }
-
+  }
   return Out.str();

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)



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1100
+  // internal linkage functions, to differentiate the symbols across
+  // modules.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68049/new/

https://reviews.llvm.org/D68049



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram created this revision.
tmsriram added a reviewer: rnk.
Herald added a project: clang.

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, , 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=: Enables all options related to Propeller for 
optimizing like basic block sections and associated linker options.


Repository:
  rC Clang

https://reviews.llvm.org/D68049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/basicblock-sections.c
  clang/test/CodeGen/basicblock-sections.funcnames
  clang/test/CodeGen/unique_internal_funcnames.c
  clang/test/Driver/propeller-flags.c
  clang/tools/driver/cc1as_main.cpp

Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -90,6 +90,7 @@
   unsigned SaveTemporaryLabels : 1;
   unsigned GenDwarfForAssembly : 1;
   unsigned RelaxELFRelocations : 1;
+  unsigned RelocateWithSymbols : 1;
   unsigned DwarfVersion;
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
@@ -236,6 +237,7 @@
   }
 
   Opts.RelaxELFRelocations = Args.hasArg(OPT_mrelax_relocations);
+  Opts.RelocateWithSymbols = Args.hasArg(OPT_mrelocate_with_symbols);
   Opts.DwarfVersion = getLastArgIntValue(Args, OPT_dwarf_version_EQ, 2, Diags);
   Opts.DwarfDebugFlags = Args.getLastArgValue(OPT_dwarf_debug_flags);
   Opts.DwarfDebugProducer = Args.getLastArgValue(OPT_dwarf_debug_producer);
@@ -361,6 +363,7 @@
   MAI->setCompressDebugSections(Opts.CompressDebugSections);
 
   MAI->setRelaxELFRelocations(Opts.RelaxELFRelocations);
+  MAI->setRelocateWithSymbols(Opts.RelocateWithSymbols);
 
   bool IsBinary = Opts.OutputType == AssemblerInvocation::FT_Obj;
   if (Opts.OutputPath.empty())
Index: clang/test/Driver/propeller-flags.c
===
--- /dev/null
+++ clang/test/Driver/propeller-flags.c
@@ -0,0 +1,13 @@
+// Check that -fpropeller flag invokes the correct options.
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-label -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_LABEL
+// RUN: %clang -### %s -target x86_64-unknown-linux -fpropeller-optimize=perf.propeller -flto=thin 2>&1 | FileCheck %s -check-prefix=CHECK_PROPELLER_OPT
+
+// CHECK_PROPELLER_LABEL: "-fbasicblock-sections=labels"
+// CHECK_PROPELLER_LABEL: "-funique-internal-funcnames"
+// CHECK_PROPELLER_LABEL: "--lto-basicblock-sections=labels"
+//
+// CHECK_PROPELLER_OPT: "-fbasicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "-funique-internal-funcnames"
+// CHECK_PROPELLER_OPT: "--propeller=perf.propeller"
+// CHECK_PROPELLER_OPT: "--lto-basicblock-sections=perf.propeller"
+// CHECK_PROPELLER_OPT: "--optimize-bb-jumps"
Index: clang/test/CodeGen/unique_internal_funcnames.c
===
--- /dev/null
+++ clang/test/CodeGen/unique_internal_funcnames.c
@@ -0,0 +1,18 @@
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+
+static int foo() {
+  return 0;
+}
+
+int (*bar()) () {
+   return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.$
Index: clang/test/CodeGen/basicblock-sections.funcnames
===
---