hoy added inline comments.
================
Comment at: llvm/tools/opt/NewPMDriver.cpp:136-138
+static cl::opt<bool> PseudoProbeForProfiling(
+ "new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+ cl::desc("Emit pseudo probes to enable PGO profile generation."));
----------------
dblaikie wrote:
> I guess this should probably have some separate testing, if it's a separate
> flag/feature? (& flag+tests could be in a separate commit)
I'm not sure there's a separate need for this switch except for being tested in
`unique-internal-linkage-names.ll`. The point of this whole patch is to place
the unique name pass before the pseudo probe pass and test it works. Hence it
sounds appropriate to me to include all changes in one patch. What do you think?
================
Comment at: llvm/tools/opt/NewPMDriver.cpp:253-258
if (DebugInfoForProfiling)
P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
true);
+ else if (PseudoProbeForProfiling)
+ P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+ false, true);
----------------
dblaikie wrote:
> Both of these might be more readably written as something like:
> ```
> P.emplace();
> P.PseudoProbeForProfiling = true;
> ```
> & similar. (you can commit the change to DebugInfoForProfiling separately
> before/after this change to make it consistent with the new one)
>
> But no big deal either way - while it makes these tidier it makes them a bit
> less consistent with the other three
That looks cleaner, but there are assertions in the constructor of `PGOOptions`
which I would not like to bypass by setting the `PseudoProbeForProfiling` field
separately.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93656/new/
https://reviews.llvm.org/D93656
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
