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

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Passes/PassBuilder.h:279-282
+  // PGO tunables.
+  std::string ProfileGenFile = "";
+  std::string ProfileUseFile = "";
+  bool RunProfileGen = false;
----------------
chandlerc wrote:
> 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?
Done.


================
Comment at: lib/Passes/PassBuilder.cpp:383-384
+                              std::string ProfileUseFile) {
+  if (!RunProfileGen && ProfileUseFile.empty())
+    return;
+
----------------
chandlerc wrote:
> 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.
Moved the check.


================
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)) {
----------------
chandlerc wrote:
> 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.
I updated the comment, and maintained the old behaviour (not running simplifications for `-Os/Oz`).
I agree this could be re-evaluated in the future.


================
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;
+
----------------
chandlerc wrote:
> 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.
I agree with you. David, would you be fine with the status quo or you feel strong about having this a `cl::opt` ? [I personally don't think we should spend too much time worrying about sharing code between old and new PM in `lib/Passes` (we already share enough where it matters, e.g. in the passes themselves).


https://reviews.llvm.org/D29308