[PATCH] D86167: [opt][NewPM] Add basic-aa in legacy PM compatibility mode

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/tools/opt/NewPMDriver.cpp:351
+  // For compatibility with legacy pass manager.
+  // basic-aa was always available in the legacy pass manager,
+  // unless -disable-basic-aa was specified.
----------------
aeubanks wrote:
> ychen wrote:
> > aeubanks wrote:
> > > ychen wrote:
> > > > aeubanks wrote:
> > > > > ychen wrote:
> > > > > > I think it is helpful to be specific here, saying something like "AAResultsWrapperPass` by default provide basic-aa in legacy PM."
> > > > > > 
> > > > > > Could we combine this code with the `if (!AAPipeline.empty())` code above? If the AAPipeline is empty, we could make it "basic-aa".
> > > > > Added comment.
> > > > > 
> > > > > I don't want to change the semantics of any tests using `opt -passes=foo`, this is purely for compatibility with tests using `opt -foo-pass`.
> > > > > 
> > > > > And actually I think having to explicitly specify basic-aa makes more sense than implicitly adding it, so I like the NPM way of doing it. But that's a different discussion.
> > > > That makes sense. Does it mean this code is temporary? i.e once we switch to NPM, we will remove this code and mass-adding `basic-aa` to relevant tests? If so, then sounds good to me.
> > > I was thinking that would happen alongside whenever we migrate tests to use '-passes='.
> > Sounds good. Please add a comment about the next step related to this code.
> > 
> > Could you check the empty AAPipeline instead? The code right now is adding `basic-aa` to AAPipeline. 
> Added comment.
> 
> `AAPipeline` has nothing to do with the legacy PM compatibility mode (there's an assert checking that it is empty if we are in the legacy PM compat mode). The code isn't adding `basic-aa` to `AAPipeline`, it's adding it to `AA`.
Oh, now I see the implicit assert above. I was looking for an assertion related to AAPipeline. Thanks. Looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86167