dblaikie 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."));
----------------
aeubanks wrote:
> hoy wrote:
> > 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?
> +1 to hoy's comment. I don't think there's a need to make patches strictly as
> incremental as possible if they're already small. (I would have been okay
> with keeping the Clang change here FWIW)
I understand I'm a bit of a stickler for some of this stuff - though the
particular reason I brought it up here is that this adds two flags, but doesn't
test them separately, only together. So it's not clear/tested as to which
behaviors are provided by which flags.
Separating the flags would make it clear that each flag/functionality was
tested fully.
Please add test coverage for each flag separately, optionally separate this
into two patches to make it clearer how each piece of functionality is tested.
================
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);
----------------
hoy wrote:
> 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.
Ah, fair enough.
Though given the struct has publicly mutable members, these assertions aren't
especially effective. Looks like they started being added here:
https://reviews.llvm.org/D36040 - which I guess doesn't tell us much, but I was
curious. Wonder if we could move the checks to somewhere more robust. (separate
patch, in any case)
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
