hoy marked an inline comment as done.
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:
> 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.
Sounds good, a separate test added for the pseudo probe flag.
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
