Page MenuHomePhabricator

[PGO] Passmanagerbuilder change that enable IR level PGO instrumentation
ClosedPublic

Authored by xur on Dec 30 2015, 1:46 PM.

Details

Summary

This patch includes the passmanagerbuilder change that enables IR level PGO instrumentation. It adds two passmanagerbuilder options:

-profile-generate=<profile_filename>
-profile-use=<profile_filename>

which is primary for debug purpose (or called directly from opt).

It adds two fields in class PassManagerBuilder:
std::string PGOInstrGen
std::string PGOInstrUse
which specifies profile name for generation and use passes, respectively.

In PassManagerBuilder::populateModulePassManager(), it adds a preinstrumenation inline pass if IR level instrumentation is enabled. It then calls the profile generation or use pass as the option specified.

Diff Detail

Repository
rL LLVM

Event Timeline

xur updated this revision to Diff 43803.Dec 30 2015, 1:46 PM
xur retitled this revision from to [PGO] Passmanagerbuilder change that enable IR level PGO instrumentation.
xur updated this object.
xur added reviewers: davidxl, silvas, bogner.
xur added a subscriber: llvm-commits.
davidxl added inline comments.Dec 30 2015, 2:54 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
208 ↗(On Diff #43803)

Is it better to do max(SizeLevel, 1) instead of SizeLevel?

mehdi_amini added inline comments.Jan 3 2016, 11:57 AM
lib/Transforms/IPO/PassManagerBuilder.cpp
281 ↗(On Diff #43803)

There will be an inliner SCC pass manager created in the PGO InstrPasses. It is not clear to me why you don't integrate this with the regular inliner a few line below.

davidxl added inline comments.Jan 3 2016, 12:42 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
281 ↗(On Diff #43803)

This is the pre-inline pass that needs to happen before instrumentation. See the original RFC: http://lists.llvm.org/pipermail/llvm-dev/2015-August/089425.html for details.

chandlerc added inline comments.
lib/Transforms/IPO/PassManagerBuilder.cpp
281 ↗(On Diff #43803)

Yes, and there was not consensus in the RFC for the pre-inlining. Instead, the approach suggested was to work on independent components which seem non-controversial and have a separate focused discussion about pre-inlining and other interactions.

I think there are still a lot of unanswered questions around the pre-inlining phase (based on my reading of the RFC thread you pointed at). If there is some other thread that I've not seen, feel free to direct there...

davidxl added inline comments.Jan 3 2016, 5:23 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
281 ↗(On Diff #43803)
slingn added a subscriber: slingn.Jan 4 2016, 9:14 AM
slingn added inline comments.
lib/Transforms/IPO/PassManagerBuilder.cpp
111 ↗(On Diff #43803)

typo: phrase -> phase

xur updated this revision to Diff 44810.Jan 13 2016, 4:07 PM

Some minor changes based on the review comments.

silvas added inline comments.Jan 15 2016, 6:09 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
216 ↗(On Diff #44810)

Let's drop the pre-instrumentation passes for now and leave a TODO. We will want to have a focused discussion on what to put here.

281 ↗(On Diff #44810)

Yes, and there was not consensus in the RFC for the pre-inlining. Instead, the approach suggested was to work on independent components which seem non-controversial and have a separate focused discussion about pre-inlining and other interactions.

Agreed. We will definitely want to have a focused discussion with real-world performance measurements when doing pass pipeline tuning (I have a big chunk of time allocated for characterizing and tuning the new IR-level instrumentation stuff and am looking forward to participating!). For the moment, just wiring it up and leaving a TODO for the pre-instrumentation passes is probably the right incremental step.

(I just got back from vacation and am still working on my email backlog, but I should be back on my feet next week and the new IR-level instrumentation stuff is my top priority at the moment)

xur updated this revision to Diff 45429.Jan 20 2016, 12:08 PM

Per Sean's suggestion, I removed the preinliner change to have the patch focused on the options only.
I will later create a separated patch to discuss preinliner change.

Thanks,

-Rong

silvas accepted this revision.Jan 20 2016, 4:04 PM
silvas edited edge metadata.

I'm not an expert in PassManagerBuilder but this makes sense to me.

This revision is now accepted and ready to land.Jan 20 2016, 4:04 PM
davidxl accepted this revision.Jan 20 2016, 4:54 PM
davidxl edited edge metadata.

LGTM with the nit.

lib/Transforms/IPO/PassManagerBuilder.cpp
211 ↗(On Diff #45429)

Add a comment : add lowering pass.

silvas added inline comments.Jan 20 2016, 6:15 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
204 ↗(On Diff #45429)

One tiny nit I just saw: this check is redundant. The function does nothing if they are both empty because they are checked below individually.

xur added a subscriber: xur.Jan 20 2016, 6:24 PM

You are right. I have this stmt here to avoid preinliner for non PGO
compilation. Now the preinliner is gone. I should have removed it. Thanks
for catching this. Will fix.

Rong

This revision was automatically updated to reflect the committed changes.