Page MenuHomePhabricator

[PM] Hook the instrumented PGO machinery in the new PM

Authored by davide on Jan 30 2017, 2:32 PM.



This is the required plumbing to add PGO to the new PM (which allows to test the changes to make the inliner profile-aware).
As long as several of us are interested in this, I decided to share it early (in the hope to save to somebody some boring typing).
I'll upload the clang bits soon, but I have some comments.

  1. The default threshold for the inliner run used to be a tunable. I just hardcoded the value of the cl::opt. If people think it's valuable to thread this from the frontend down to the optimizer, happy to (but I wouldn't keep the cl::opt around). My take is that this option shouldn't be exposed.
  2. There seems to be a comment about the hint threshold of the inliner. I blatantly cargo culted that from the old PM, but maybe should be removed ? (or we should get around to do the required performance testing to tune it)


Diff Detail


Event Timeline

davide created this revision.Jan 30 2017, 2:32 PM
davidxl added inline comments.Jan 31 2017, 3:54 PM
394 ↗(On Diff #86349)

can you refactor the old pm code such that the existing internal options can be reused here?

487 ↗(On Diff #86349)

this seems like exposing a bug in old implementation as well. IndirectCallPromotion pass need not to be run profile-use is in effect. Otherwise it is a waste of compile time.

davide added inline comments.Jan 31 2017, 4:45 PM
394 ↗(On Diff #86349)

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::opts ?

487 ↗(On Diff #86349)

Will submit a patch separately. It's not gonna be a substantial reduction in compile time as this pass is very fast, but still there's no point in running it.

chandlerc added inline comments.Feb 8 2017, 10:38 AM
279–282 ↗(On Diff #86349)

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?

383–384 ↗(On Diff #86349)

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.

386–390 ↗(On Diff #86349)

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.

394 ↗(On Diff #86349)

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.

Your comments are reasonable, I'll address them soon.
@davidxl, @rongxu, @eraman what's the rationale behind not running the pre-inliner and simplification passes when optimizing for size?
My understanding is that we run them to cleanup and avoid putting "too many counters" (which would slow down the instrumentation) [classical example operator +].
Am I correct or is there any other reason?

Actually, @xur, not @rongxu

davide updated this revision to Diff 87712.Feb 8 2017, 2:25 PM
davide marked 2 inline comments as done.

Updated + comments. I'll strip the call to indirectcallpromotion in the !PGO case as a follow-up.

279–282 ↗(On Diff #86349)


383–384 ↗(On Diff #86349)

Moved the check.

386–390 ↗(On Diff #86349)

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.

394 ↗(On Diff #86349)

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).

davidxl added inline comments.Feb 8 2017, 2:38 PM
394 ↗(On Diff #86349)

keeping it as is is fine -- we can revisit this later if we want.

chandlerc accepted this revision.Feb 10 2017, 11:22 AM

LGTM with a minor tweak above, feel free to just submit with that change unless you'd like to do something different.

135–136 ↗(On Diff #87712)

Pass and store an optional rather than a pointer? Removes lifetime concerns and ownersihp questions, and this seems a not-scary data type to copy.

This revision is now accepted and ready to land.Feb 10 2017, 11:22 AM
This revision was automatically updated to reflect the committed changes.