[PATCH] D35807: Add test coverage for new PM PGOOpt handling.

[Dehao Chen via Phabricator via llvm-commits]

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


================
Comment at: lib/Passes/PassBuilder.cpp:175-178
+static cl::opt<std::string> PGOOptString(
+    "pgo-opt", cl::init(""), cl::Hidden,
+    cl::desc("PGOOpt string in the format of "
+             "{gen|use|sample_gen|sample_use}=value"));
----------------
chandlerc wrote:
> Rather than these options going in PassBuilder.cpp and overriding things, I think this should go into tools/opt/NewPMDriver.cpp much like the ThinLTO controls do.
> 
> Then the opt tool can construct a PGOOpt struct and hand it into the PassBuilder much the same way Clang will.
> 
> Also, `sample_gen` doesn't appear to be valid?
> 
> I think rather than having one flag that you have to parse I would suggest have three flags: `pgo-instr-gen`, `pgo-intsr-use`, and `pgo-sample-use`. I think there is a way in the `cl::opt` layer to say the flags are mutually incompatible, but even if you have to do that manually, it seems better than parsing an '=' out of the flag value.
pgo-instr-{gen|use}, conflict with existing flags. Used pgo-{gen|use|sample-use} instead.


https://reviews.llvm.org/D35807