[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", "<filename>", "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<f_Group>, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : all | labels | none | <filename>">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group<f_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=<filename>
> 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 | <filename>">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group<f_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