Page MenuHomePhabricator

[HotColdSplit] Schedule splitting late to fix perf regression

Authored by vsk on Feb 14 2019, 3:25 PM.



With or without PGO data applied, splitting early in the pipeline
(either before the inliner or shortly after it) regresses performance
across SPEC variants. The cause appears to be that splitting hides
context for subsequent optimizations.

Schedule splitting late again, in effect reversing r352080, which
scheduled the splitting pass early for code size benefits (documented in


Event Timeline

vsk created this revision.Feb 14 2019, 3:25 PM
tejohnson added inline comments.Feb 14 2019, 4:36 PM
764 ↗(On Diff #186952)

I see in the old PM that splitting is being moved post link for full LTO as well. IMO it should be in roughly the same place for the new PM. I guess the issue here is that there is no indication at this point that we are doing full LTO pre-link vs non-LTO. I would say either keep it pre-link in both PMs for full LTO or maybe better to pass down some indication that we are in the full LTO pre-link and not do it here in that case either (and then add it to buildLTODefaultPipeline as well to get it enabled for full LTO post-link). I'm reviewing another patch that needs to know this as well (D54175) and is adding a "bool LTOPreLink" parameter to this method. So maybe use the same thing?

5 ↗(On Diff #186952)

Can you add a regular LTO postlink invocation as well? That would be --passes='lto<Os'

vsk added a subscriber: xur.Feb 14 2019, 4:43 PM
vsk added inline comments.
764 ↗(On Diff #186952)

I'm not sure whether there's an advantage to scheduling splitting twice (as in, I haven't tested this out), but there's at least a compile-time argument for only doing it once. @xur would you be ok with committing your bool LTOPreLink change from D54175 early? Happy to send in patch for this if you'd prefer.

tejohnson added inline comments.Feb 14 2019, 4:54 PM
764 ↗(On Diff #186952)

I don't think we should do it twice, it should only be done once. The issue is that it is being added in different places in each PM with this patch. So my suggestion is to either move it post link here or pre link in the other PM.

@xur is out for a couple weeks, so if you want to add that part in the meantime that would be great!

vsk updated this revision to Diff 186952.Feb 14 2019, 5:23 PM
vsk marked 4 inline comments as done.
  • Add a NewPM LTO pre-link test.
  • Schedule splitting at the same point in the old & new PM's.
This revision is now accepted and ready to land.Feb 14 2019, 6:51 PM
This revision was automatically updated to reflect the committed changes.