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

[David Blaikie via Phabricator via llvm-commits]

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