This is an archive of the discontinued LLVM Phabricator instance.

[NewPM][opt] Run the "default" AA pipeline by default
ClosedPublic

Authored by aeubanks on Jan 20 2021, 11:52 PM.

Details

Summary

We tend to assume that the AA pipeline is by default the default AA
pipeline and it's confusing when it's empty instead.

PR48779

Diff Detail

Event Timeline

aeubanks created this revision.Jan 20 2021, 11:52 PM
aeubanks requested review of this revision.Jan 20 2021, 11:52 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Could we have at least one test showing how to pass an empty pipeline, solely for testing purposes?
AFAICT, currently all tests use the 'default', 'basic-aa' or 'tbaa, basic-aa'. Potentially add the test to test/Other/pass-pipeline-parsing.ll, which is testing 'bad' AA pipeline, or in one of the AA test folders.

aeubanks updated this revision to Diff 318245.Jan 21 2021, 9:41 AM

add tests

asbirlea accepted this revision.Jan 21 2021, 1:08 PM

lgtm.

This revision is now accepted and ready to land.Jan 21 2021, 1:08 PM
This revision was landed with ongoing or failed builds.Jan 21 2021, 7:47 PM
This revision was automatically updated to reflect the committed changes.
markus added a subscriber: markus.Jan 22 2021, 8:23 AM
markus added inline comments.
llvm/tools/opt/NewPMDriver.cpp
351

Realizing I am a bit late to the party but this change here confuses me.

Previously in https://reviews.llvm.org/D82488?id=273164#inline-758886 it was requested that "Could you assert AAPipeline and AA passes inside Passes don't co-exist?" but the assert added at that point was actually stronger making sure that Passes contains no passes at all.

While I guess this code is equivalent to its predecessor in that sense I don't quite understand why this limitation is needed. Especially as it gets in the way for our use case.

E.g. if I run

opt -enable-new-pm -loop-vectorize input.ll

I do not get any "default" AA at all.

Could of course be our use case is unintended and dumb but still … :)

aeubanks added inline comments.Jan 22 2021, 9:31 AM
llvm/tools/opt/NewPMDriver.cpp
351

If you want to use the new PM, you should use the -passes=loop-vectorize flag, not -loop-vectorize.
-enable-new-pm is strictly for the new PM transition, not intended as a long term thing.

bjope added a subscriber: bjope.Jan 28 2021, 4:36 AM
bjope added inline comments.
llvm/tools/opt/NewPMDriver.cpp
364

Isn't the default in the legacy PM "default" rather than "basic-aa"?

I think that by using "default" here the backwards compatibility would be more correct (and the example given by @markus above would yield same result as without -enable-new-pm). I'd be happy to create a patch if that sounds reasonable.

aeubanks added inline comments.Jan 28 2021, 9:54 AM
llvm/tools/opt/NewPMDriver.cpp
364

The legacy PM defaults to only having basic-aa by default, see AAResultsWrapperPass::runOnFunction(). Something like -O2 will add other alias analyses to the pipeline, see PassManagerBuilder::addInitialAliasAnalysisPasses() and other places where PassManagerBuilder will add extra AA analyses.

I don't see a reason to upgrade all tests that are using the opt -instcombine syntax to run the default set of alias analyses rather than just basic-aa. I'd rather not change tests while changing the value of -enable-new-pm since this affects almost every test. Incrementally fixing up tests to only use opt -passes=instcombine will change the AA pipeline that they use (if the test doesn't manually specify its own), but I think that's ok.

bjope added inline comments.Jan 28 2021, 1:09 PM
llvm/tools/opt/NewPMDriver.cpp
364

Ah, so my problem was actually when running opt -enable-new-pm -O1 and comparing result with opt -O1. So the "default" in legacy PM is different depending on if running a named passes or using O1 or higher. So I either need to change test drivers etc to use -passes for everything (we do lots of fuzzy testing with random passes and -O<n> for n>0), or I need to add -enable-new-pm -aa-pipeline=default to the command line whenever I also have -O1 etc (to get similar results as with legacy PM).

Or to get "compatibility with the legacy PM AA pipeline" we should check if OptLevel>0 is used and then conditionally select between "basic-aa" and "default" here.

aeubanks added inline comments.Jan 28 2021, 1:15 PM
llvm/tools/opt/NewPMDriver.cpp
364

I'd say use -passes for everything since the new PM will be the default very soon. Again, -enable-new-pm is strictly a transition flag, not intended for long term use.
-enable-new-pm -aa-pipeline=foo shouldn't work due to the if (Passes.empty()) check above