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

David Li via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 8 14:38:56 PST 2017


davidxl added inline comments.


================
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:
> 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).
keeping it as is is fine -- we can revisit this later if we want.


https://reviews.llvm.org/D29308





More information about the llvm-commits mailing list