[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<f_Group>;
+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>">;
----------------
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<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>,
----------------
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<f_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<f_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<llvm::BasicBlockSection>(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