[PATCH] D17737: [PGO] change profile use cc1 option

[Sean Silva via cfe-commits]

silvas added a comment.

Thanks, this is looking good. I have some naming suggestions and nits, but the overall content of the patch LGTM.


================
Comment at: include/clang/Driver/CC1Options.td:286
@@ +285,3 @@
+def fprofile_instrument_usepath_EQ :
+    Joined<["-"], "fprofile-instrument-usepath=">,
+    HelpText<"Specify the profile path in PGO use compilation">;
----------------
Small bikeshed: `usepath` reads weird to me. Can we make it `use-path`?

================
Comment at: include/clang/Frontend/CodeGenOptions.h:237
@@ +236,3 @@
+  /// \brief Check if Clang profile use is on.
+  bool hasProfileClangUse () const {
+    return getProfileUse() == ProfileClangInstr;
----------------
No space before the parens. Here and elsewhere.

================
Comment at: lib/Frontend/CompilerInvocation.cpp:378
@@ -377,1 +377,3 @@
 
+enum PGOInstrumentor { ProfileUnknown, ProfileNone, ProfileClang, ProfileLLVM };
+static PGOInstrumentor getPGOInstrumentor(StringRef S) {
----------------
Can we reuse the enum used for CodeGenOpts? Please add a comment if not.

================
Comment at: lib/Frontend/CompilerInvocation.cpp:533
@@ +532,3 @@
+      Args.getLastArgValue(OPT_fprofile_instrument_path_EQ);
+  Opts.InstrProfileInput =
+      Args.getLastArgValue(OPT_fprofile_instrument_usepath_EQ);
----------------
Maybe consider renaming these variables in a separate patch. The current naming is pretty confusing since it makes it seems like InstrProfileOutput mirrors InstrProfileInput, but they are totally unrelated. Maybe `InstrProfileDefaultProfrawPath` and `InstrProfileUsePath`, respectively.

Maybe in this patch rename `InstrProfileInput` to `ProfileInstrumentUsePath` to match the option name?


http://reviews.llvm.org/D17737