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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks 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.
----------------
asbirlea wrote:
> ychen wrote:
> > 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.
> I honestly don't like the idea of doing this now just for the tests.
> 
> We've had out-of-tree testing in the community that relied on the -disable-aa flag, and on other particular command-line behavior for fuzz testing. This patch is changing the current NPM expected behavior.
> 
> We either change the behavior in this patch and stick to it, or we update the tests at this point and never change the behavior in the first place.
> I think ping-ponging with expectations just to delay test changes is not a positive experience for such users.
Keeping the behavior in this patch going forward does sound better.
My main intention was that if we ever get rid of the `opt -foo-pass` syntax and migrate relevant tests to use `opt -passes=foo -aa-pipeline=basic-aa`, we can delete this. Not that we'd go back and add `-basic-aa` to `opt -foo-pass` tests later.

Maybe the comment I added isn't clear, but that's what I meant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86167