Page MenuHomePhabricator

[PM] Flesh out the new pm LTO pipeline
ClosedPublic

Authored by davide on Jan 22 2017, 3:54 PM.

Details

Summary

This is pretty much a carbon copy of the current pass manager pipeline, and it unblocks my testing. Passes not yet ported or disabled because of problems with the loop PM are marked with a FIXME. So, not complete, but hopefully a starting point. For those watching, this is so that I can start playing with PGO support in the new inliner (at LTO time).

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Jan 22 2017, 3:54 PM

Note: this needs some tests associated, but before spending time on that I want to make sure we agree with the general direction.

What is the justification instead of adopting the ThinLTO pipeline?

What is the justification instead of adopting the ThinLTO pipeline?

The two problems are largely orthogonal. This is a port of the old LTO pipeline to the new pass manager. The aim is to make sure the current LTO pipeline works correctly with the new pass manager. I prefer to keep the old functionality in the new pass manager until it stabilizes. After that (or in parallel), we can decide whether we want to unify the two pipelines (if it's possible/reasonable).
On top of that, currently plugging the ThinLTO pipeline for LTO results in 2x/2.5x increase in compile time. Definitely not an hit I want to pay.

What is the justification instead of adopting the ThinLTO pipeline?

The two problems are largely orthogonal. This is a port of the old LTO pipeline to the new pass manager.

Apologies, I totally misunderstood what you were doing here, I thought this was the followup to our discussion last week about revisiting the LTO PM.

LGTM.
(Not sure about testing, may not be worth it at this point).

lib/Passes/PassBuilder.cpp
553 ↗(On Diff #85304)

Alias analyses are added one layer above IIRC?

575 ↗(On Diff #85304)

This seems dead?

619 ↗(On Diff #85304)

It seems that there is "PruneEH" in the legacy PM here?

mehdi_amini accepted this revision.Jan 22 2017, 5:15 PM
This revision is now accepted and ready to land.Jan 22 2017, 5:15 PM
davide added inline comments.Jan 22 2017, 5:56 PM
lib/Passes/PassBuilder.cpp
553 ↗(On Diff #85304)

If you mean, in the caller of buildLTODefaultPipeline, then answer is yes (see my other patch for lib/LTO.

575 ↗(On Diff #85304)

Yes, nice catch. I originally created this one as I thought we were running multiple CGSCC passes, but it turns out we just run PostOrderFunctionAttrs so that can be folded into the only argument of createModuleToPostOrderCGSCCPassAdaptor().

619 ↗(On Diff #85304)

We used to. But it wasn't ported to the new PM and Chandler is not entirely convinced it's worth running this anymore (see the comment in the perModulePipeline).
Happy to duplicate the comment here, or add a FIXME.
Side note: I have no strong opinion whether we should run the pass or not.

chandlerc edited edge metadata.Jan 22 2017, 6:36 PM

I would suggest basic testing here similar to what we do for O[1-3sz] in test/Other/new-pm-defaults.ll. I'd keep the pre-link-LTO and non-LTO in that file and add a new file (new-pm-lto-defaults.ll) for the post-link LTO pipeline.

lib/Passes/PassBuilder.cpp
672 ↗(On Diff #85304)

Typo here: assumptions -> Assumptions

davide updated this revision to Diff 85421.Jan 23 2017, 10:30 AM

Added tests as per Chandler's suggestion and addressed Mehdi's comments.

chandlerc added inline comments.Jan 23 2017, 2:43 PM
test/Other/new-pm-lto-defaults.ll
22 ↗(On Diff #85421)

Why not use -NEXT checks the way mkuper made the main one? Seems much more robust at detecting changes and making sure they are intentional.

davide updated this revision to Diff 85463.Jan 23 2017, 3:03 PM

Use CHECK-NEXT for more robust testing as recommended by Chandler.

chandlerc accepted this revision.Jan 23 2017, 5:03 PM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.