[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

Thu Dec 31 11:44:29 PST 2020

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