[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>();
----------------
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.


================
Comment at: llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp:168
     AU.addRequired<AAResultsWrapperPass>();
-    AU.addPreserved<AAResultsWrapperPass>();
     AU.addRequired<ScalarEvolutionWrapperPass>();
----------------
foad wrote:
> Did you try the alternative fix of preserving more stuff instead of less? I.e. preseve everything that is transitively required by AAResultsWrapperPass? Is that hard to do?
I don't know if for example DominatorTree is preserved here (I imagined that someone has decided to require but not preserve DominatorTreeWrapperPass here in the past). It felt more safe (a more defensive bugfix) to drop preservation of AAResults.


================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:550
     AU.addPreserved<GlobalsAAWrapperPass>();
-    AU.addPreserved<AAResultsWrapperPass>();
   }
----------------
foad wrote:
> Ditto.
I did try some different solutions here, but never really figured out exactly what was needed to avoid the assert in setLastUser (also knowing that those transitive analyses actually are preserved).
The solution of not preserving AAResults was inspired by the change above for HexagonLoopIdiomRecognition, but it also matches what it looks like in GVN and NewGVN etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94138