[PATCH] D95117: [NewPM][opt] Run the "default" AA pipeline by default

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/tools/opt/NewPMDriver.cpp:364
   if (!Passes.empty() && !DisableBasicAA) {
     if (auto Err = PB.parseAAPipeline(AA, "basic-aa")) {
       errs() << Arg0 << ": " << toString(std::move(Err)) << "\n";
----------------
bjope wrote:
> aeubanks wrote:
> > bjope wrote:
> > > 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.
> > 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.
> 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.
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95117/new/

https://reviews.llvm.org/D95117