[PATCH] D94138: Require chained analyses in BasicAA and AAResults to be transitive

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:886
   AU.setPreservesAll();
-  AU.addRequired<BasicAAWrapperPass>();
-  AU.addRequired<TargetLibraryInfoWrapperPass>();
+  AU.addRequiredTransitive<BasicAAWrapperPass>();
+  AU.addRequiredTransitive<TargetLibraryInfoWrapperPass>();
----------------
bjope wrote:
> foad wrote:
> > I think this makes sense because a BasicAAResult object can be added to the AAResults object. But what about all the other kinds of AA object that can (optionally) be added? E.g. if we add a SCEVAAResult, doesn't that mean that this pass needs to "transitively require if available" SCEVAAWrapperPass?
> Right. I don't really now how it works for addUsedIfAvailable (there is no addUsedIfAvailableTransitive). We have the same thing in BasicAliasAnalysis that uses PhiValuesWrapperPass if available.
> 
> I'm also a bit unsure about if we should do something with getAAResultsAnalysisUsage at the end of this file as well. That one does not even mention that it require BasicAA, even though createLegacyPMAAResults may add a BasicAA depending on DisableBasicAA.
The RequiredTransitive set is only used in one place afaict. Not sure, but maybe we can remove addRequiredTransitive completely and treat all nested required analyses as being requested transitively in setLastUser instead. I don't understand why one would need to choose between addRequired and addRequiredTransitive if one need to use the latter as soon as there are chained analyses (or if there is a use case when it would be wrong/bad to use addRequiredTransitive instead of addRequired.

I think that the above might be worth investigating in a follow-up to this more prioritized bugfix.

Another thing I noticed is that the PreservedSet, RequiredSet etc. are implemented using vectors, and when doing getPreservedSet one could end up getting a vector with duplicates (not really a set). So verifyPreservedAnalysis could end up verifying the same analysis several times. Kind of stupid and a waste of time afaict. This is also something that I see as a candidate for a follow-up commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94138