[PATCH] D29308: [PM] Hook the instrumented PGO machinery in the new PM

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/Passes/PassBuilder.h:279-282
+  // PGO tunables.
+  std::string ProfileGenFile = "";
+  std::string ProfileUseFile = "";
+  bool RunProfileGen = false;
----------------
I think we should extract this to a PGO option struct that is passed into the passBuilder rather than being public. What do you think?


================
Comment at: lib/Passes/PassBuilder.cpp:383-384
+                              std::string ProfileUseFile) {
+  if (!RunProfileGen && ProfileUseFile.empty())
+    return;
+
----------------
Make this an assert and only call this routine when the conditions are met? It seems nicer to have the predicate in the caller so that the caller understands what enables or doesn't this pipeline.


================
Comment at: lib/Passes/PassBuilder.cpp:386-390
+  // In the old pass manager we ran the preinline and cleanup passes only at
+  // -O1 and above. There's no such thing as running optimizations at -O0 in
+  // the new pass manager, so we can skip that.
+  // We skip this step if we're optimizing for size.
+  if (!isOptimizingForSize(Level)) {
----------------
I dunno why we would want to change this when optimizing for size really.... I mean, maybe disable this under Oz?

I'd write the comment about why it is disabled, not about the old pass manager.


================
Comment at: lib/Passes/PassBuilder.cpp:394
+    // In the old pass manager, this is a cl::opt. Should still this be one?
+    IP.DefaultThreshold = 75;
+
----------------
davide wrote:
> davidxl wrote:
> > can you refactor the old pm code such that the existing internal options can be reused here?
> I'm under the impression that several (me included) dislike the use of `cl::opt` for the new pass manager. Do you have any suggestions on how to handle this haring without sharing the `cl::opt`s ?
I'm happy to have a `cl::opt` interface for things that we only expect to use as developers when testing / experimenting and we don't expect to support.

Inliner thresholds seem like a good example of this: we don't actually (IMO) want to expose a supported interface for arbitrarily changing the threshold because we're not going to test / support arbitrary thresholds. But we *do* want to be able to run experiments, etc.

However, this particular threshold seems an even less interesting thing to tune as it is chose more based on the nature of instrumentation rather than on any performance characteristics... So I don't feel strongly about whether it is a constant or a `cl::opt`.


As to whether we should share these, I again don't feel strongly. I'm happy to have two cl::opt flags if that's more convenient because, again, they're an internal interface used for development. However, if that's causing issues, we can add a helper function to the inliner along the lines of `getPGOPreInlineParams()` and sink these values (and their `cl::opt` flags) into that implementation so it can be shared between the two PMs.


https://reviews.llvm.org/D29308