This is an archive of the discontinued LLVM Phabricator instance.

[PM/AA] Teach the new pass manager to use pass-by-lambda for registering analysis passes, support pre-registering analyses, and use that to implement parsing and pre-registering a custom alias analysis pipeline.
ClosedPublic

Authored by chandlerc on Feb 15 2016, 2:22 AM.

Details

Summary

With this its possible to configure the particular alias analysis
pipeline used by the AAManager from the commandline of opt. I've updated
the test to show this effectively in use to build a pipeline including
basic-aa as part of it.

My big question for reviewers are around the APIs that are used to
expose this functionality. Are folks happy with pass-by-lambda to do
pass registration? Are folks happy with pre-registering analyses as
a way to inject customized instances of an analysis while still using
the registry for the general case?

Other thoughts of course welcome. The next round of patches will be to
add the rest of the alias analyses into the new pass manager and wire
them up here so that they can be used from opt. This will require
extending the (somewhate limited) functionality of AAManager w.r.t.
module passes.

Diff Detail

Event Timeline

chandlerc updated this revision to Diff 47964.Feb 15 2016, 2:22 AM
chandlerc retitled this revision from to [PM/AA] Teach the new pass manager to use pass-by-lambda for registering analysis passes, support pre-registering analyses, and use that to implement parsing and pre-registering a custom alias analysis pipeline..
chandlerc updated this object.
chandlerc added a subscriber: llvm-commits.
bkramer added inline comments.
include/llvm/Passes/PassBuilder.h
101

s/provide/&d/

lib/Passes/PassBuilder.cpp
108

What's being captured by the lambda here?

chandlerc added inline comments.Feb 15 2016, 11:51 AM
lib/Passes/PassBuilder.cpp
108

There are members of PassBuilder that may be used when constructing passes.

pete added a subscriber: pete.Feb 15 2016, 12:30 PM

So i'm not against the lambda's if they are required, but I can't really see why we need them instead of say, a function pointer?

Eg, instead of

MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });

do

MAM.registerPass(&FunctionAnalysisManagerModuleProxy)

Also, looks like the lambda (or function pointer), is being used to make sure we don't construct a pass twice. Why not jus make that an error to register the same pass twice? Is that something we can say is a condition of using the new pass manager?

In D17259#352996, @pete wrote:

So i'm not against the lambda's if they are required, but I can't really see why we need them instead of say, a function pointer?

Eg, instead of

MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });

do

MAM.registerPass(&FunctionAnalysisManagerModuleProxy)

This doesn't compile because that isn't a function. It is a constructor that builds a temporary.

The routine however *would* be fine with a function pointer if we had such a thing. The routine is being generic and accepting any callable. Callers can use a function pointer or a lambda or whatever else. A lambda has the nice property of being a maximally generic way to pass in a lazy-evaluated expression.

Also, looks like the lambda (or function pointer), is being used to make sure we don't construct a pass twice. Why not jus make that an error to register the same pass twice? Is that something we can say is a condition of using the new pass manager?

Well, the previous code did exactly that. ;] The patch description (i thought) explained why I am changing it: we need some way to explicitly handle the AAManager's registration outside of the generic registerFunctionAnalyses routine. The best idea I had for an interface was to pre-register the analyses which require custom logic, and then register the remaining ones with the generic logic in PassBuilder::register*Analyses.

The code to use this facility is also included in the patch. See the code in NewPMDriver.cpp where we call registerFunctionAnalyses. If you add back the assert that this patch removes from registerPass, it will fire because we will register AAManager twice.

Any further thoughts here? I don't want this to get bogged down, as it seems pretty simple and is in my critical path.

reames added a subscriber: reames.Feb 16 2016, 7:30 PM

Have to admit, I don't really understand the motivation for this. Having said that, I don't see anything obviously wrong, and I don't want you blocked on this, so LGTM.

lib/Passes/PassBuilder.cpp
224

Add an #undef after.

Have to admit, I don't really understand the motivation for this. Having said that, I don't see anything obviously wrong, and I don't want you blocked on this, so LGTM.

Well, I think its important that the motivation makes sense. The key thing is that we want command line tools like 'opt' to be able to customize the AAManager (potentially using the PassBuilder to do so!), and use that customized AAManager instead of whatever the default would be while still getting the default registration of every other analysis. The mechanism I ended up with was to allow callers of registerFunctionAnalyses to pre-register analyses if they desire.

lib/Passes/PassBuilder.cpp
224

The .def file should be #undefing everything it uses. I'll make sure I got that right (I think I didn't).

pete added a comment.Feb 16 2016, 8:01 PM

Thanks for the explanation. Looks like I should mostly have read it better the first time, but I'm more or less up to speed now.

If you wanted a LGTM from me too then you have it. Wasn't sure if you wanted an ok or not after Philip's LGTM :)

Cheers
Pete

mcrosier accepted this revision.Feb 17 2016, 6:43 AM
mcrosier added a reviewer: mcrosier.
mcrosier edited edge metadata.

LGTM, per Pete and Philip.

This revision is now accepted and ready to land.Feb 17 2016, 6:44 AM
chandlerc updated this revision to Diff 48287.Feb 18 2016, 1:45 AM
chandlerc marked an inline comment as done.
chandlerc edited edge metadata.

Fix typo spotted by Ben.

Thanks all, landing. Please let me know Philip if there is more to explain here. I'm happy to revisit this design and other aspects of this as needed.

This revision was automatically updated to reflect the committed changes.