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

[Hongtao Yu via Phabricator via llvm-commits]

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