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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
