[AliasAnalysis] AAResults preserves AAManager.

[asbirlea]

Summary

AAResults should not invalidate AAManager.
Update tests.

[chandlerc]

This looks right to me. Some suggestions on comment wording only.

lib/Analysis/AliasAnalysis.cpp

82–83 ↗

(On Diff #195898)

One nit about wording "Do not invalidate it here." -> "No need to check whether it has been preserved explicitly."

You should also update the comments on AAManager to explain this, and further add the (largely already in place) restriction that the registration functions must not be called once the AAManager is run.

I would also, merge this into the comment below as a single bit of prose. Could just continue after this comment with: "However, we still need to check if any of the dependencies end up being invalidated, and invalidate ourselves in that case."

[asbirlea]

You should also update the comments on AAManager to explain this, and further add the (largely already in place) restriction that the registration functions must not be called once the AAManager is run.

Could you please elaborate on this? How/where is this restriction enforced?

[chandlerc]

In D60914#1473167, @asbirlea wrote:

You should also update the comments on AAManager to explain this, and further add the (largely already in place) restriction that the registration functions must not be called once the AAManager is run.

Could you please elaborate on this? How/where is this restriction enforced?

It's not enforced at all, that's why we should document it.... We could add enforcement too, but it is a difficult mistake to make, and so I'm not worried about it. (Explained in person the nature of the "issue".)

[asbirlea]

Address comment.

[chandlerc]

LGTM

include/llvm/Analysis/AliasAnalysis.h

1100–1102 ↗

(On Diff #196103)

This would be a bit easier for me to read as:

The result of this analysis are only invalidated if one of the
particular aggregated AA results end up being invalidated. This
removes the need to explicitly preserve the results of `AAManager`.
Note that analyses should no longer be registered once the
`AAManager` is run.

llvm/trunk/include/llvm/Analysis/AliasAnalysis.h

 /// that dispatches across all of the alias analysis results available.
 ///
 /// Note that the order in which analyses are registered is very significant.
 /// That is the order in which the results will be aggregated and queried.
 ///
 /// This manager effectively wraps the AnalysisManager for registering alias
 /// analyses. When you register your alias analysis with this manager, it will
 /// ensure the analysis itself is registered with its AnalysisManager.
+///
+/// The result of this analysis is only invalidated if one of the particular
+/// aggregated AA results end up being invalidated. This removes the need to
+/// explicitly preserve the results of `AAManager`. Note that analyses should no
+/// longer be registered once the `AAManager` is run.
+
 class AAManager : public AnalysisInfoMixin<AAManager> {
 public:
   using Result = AAResults;
 
   /// Register a specific AA result.
   template <typename AnalysisT> void registerFunctionAnalysis() {
     ResultGetters.push_back(&getFunctionAAResultImpl<AnalysisT>);
   }

llvm/trunk/lib/Analysis/AliasAnalysis.cpp

 #if 0
   for (auto &AA : AAs)
     AA->setAAResults(nullptr);
 #endif
 }
 
 bool AAResults::invalidate(Function &F, const PreservedAnalyses &PA,
                            FunctionAnalysisManager::Invalidator &Inv) {
-  // Check if the AA manager itself has been invalidated.
-  auto PAC = PA.getChecker<AAManager>();
-  if (!PAC.preserved() && !PAC.preservedSet<AllAnalysesOn<Function>>())
-    return true; // The manager needs to be blown away, clear everything.
-
-  // Check all of the dependencies registered.
+  // AAResults preserves the AAManager by default, due to the stateless nature
+  // of AliasAnalysis. There is no need to check whether it has been preserved
+  // explicitly. However, we still need to check if any of the dependencies end
+  // up being invalidated, and invalidate ourselves in that case.
   for (AnalysisKey *ID : AADeps)
     if (Inv.invalidate(ID, F, PA))
       return true;
 
   // Everything we depend on is still fine, so are we. Nothing to invalidate.
   return false;
 }
 

llvm/trunk/test/Analysis/MemoryDependenceAnalysis/invalidation.ll

 ; Test that memdep gets invalidated when the analyses it depends on are
 ; invalidated.
 ;
-; Check AA specifically.
+; Check AA. AA is stateless, there's nothing to invalidate.
 ; RUN: opt -disable-output -debug-pass-manager -aa-pipeline='basic-aa' %s 2>&1 \
 ; RUN:     -passes='require<memdep>,invalidate<aa>,gvn' \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-AA-INVALIDATE
 ; CHECK-AA-INVALIDATE: Running pass: RequireAnalysisPass
 ; CHECK-AA-INVALIDATE: Running analysis: MemoryDependenceAnalysis
 ; CHECK-AA-INVALIDATE: Running pass: InvalidateAnalysisPass
-; CHECK-AA-INVALIDATE: Invalidating analysis: AAManager
-; CHECK-AA-INVALIDATE: Invalidating analysis: MemoryDependenceAnalysis
+; CHECK-NOT-AA-INVALIDATE: Invalidating analysis: MemoryDependenceAnalysis
 ; CHECK-AA-INVALIDATE: Running pass: GVN
-; CHECK-AA-INVALIDATE: Running analysis: MemoryDependenceAnalysis
+; CHECK-NOT-AA-INVALIDATE: Running analysis: MemoryDependenceAnalysis
 ;
 ; Check domtree specifically.
 ; RUN: opt -disable-output -debug-pass-manager %s 2>&1 \
 ; RUN:     -passes='require<memdep>,invalidate<domtree>,gvn' \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-DT-INVALIDATE
 ; CHECK-DT-INVALIDATE: Running pass: RequireAnalysisPass
 ; CHECK-DT-INVALIDATE: Running analysis: MemoryDependenceAnalysis
 ; CHECK-DT-INVALIDATE: Running pass: InvalidateAnalysisPass

llvm/trunk/test/Other/new-pass-manager.ll

 ; CHECK-AA-MODULE-INVALIDATE: Starting llvm::Module pass manager run
 ; CHECK-AA-MODULE-INVALIDATE: Running pass: RequireAnalysisPass
 ; CHECK-AA-MODULE-INVALIDATE: Running analysis: GlobalsAA
 ; CHECK-AA-MODULE-INVALIDATE: Starting llvm::Function pass manager run
 ; CHECK-AA-MODULE-INVALIDATE: Running pass: RequireAnalysisPass
 ; CHECK-AA-MODULE-INVALIDATE: Running analysis: AAManager
 ; CHECK-AA-MODULE-INVALIDATE: Finished llvm::Function pass manager run
 ; CHECK-AA-MODULE-INVALIDATE: Running pass: InvalidateAnalysisPass
-; CHECK-AA-MODULE-INVALIDATE: Invalidating analysis: AAManager
 ; CHECK-AA-MODULE-INVALIDATE: Invalidating analysis: GlobalsAA
 ; CHECK-AA-MODULE-INVALIDATE: Running pass: RequireAnalysisPass
 ; CHECK-AA-MODULE-INVALIDATE: Running analysis: GlobalsAA
 ; CHECK-AA-MODULE-INVALIDATE: Starting llvm::Function pass manager run
 ; CHECK-AA-MODULE-INVALIDATE: Running pass: AAEvaluator
-; CHECK-AA-MODULE-INVALIDATE: Running analysis: AAManager
 ; CHECK-AA-MODULE-INVALIDATE: Finished llvm::Function pass manager run
 ; CHECK-AA-MODULE-INVALIDATE: Finished llvm::Module pass manager run
 
 ; RUN: opt -disable-output -disable-verify -debug-pass-manager %s 2>&1 \
 ; RUN:     -passes='require<memdep>' \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-MEMDEP
 ; CHECK-MEMDEP: Starting llvm::Module pass manager run
 ; CHECK-MEMDEP: Running pass: RequireAnalysisPass

llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

TEST_F(LoopPassManagerTest, InvalidationOfBundledAnalyses) {
   // No need to re-run if we require again from a fresh loop pass manager.
   FPM.addPass(createFunctionToLoopPassAdaptor(
       RequireAnalysisLoopPass<MockLoopAnalysisHandle::Analysis>()));
 
   // Preserving everything but the loop analyses themselves results in
   // invalidation and running.
   EXPECT_CALL(MFPHandle, run(HasName("f"), _))
       .WillOnce(Return(getLoopPassPreservedAnalyses()));
-  EXPECT_CALL(MLAHandle, invalidate(_, _, _)).Times(3);
   EXPECT_CALL(MLAHandle, run(HasName("loop.0.0"), _, _));
   EXPECT_CALL(MLAHandle, run(HasName("loop.0.1"), _, _));
   EXPECT_CALL(MLAHandle, run(HasName("loop.0"), _, _));
   FPM.addPass(MFPHandle.getPass());
   FPM.addPass(createFunctionToLoopPassAdaptor(
       RequireAnalysisLoopPass<MockLoopAnalysisHandle::Analysis>()));
 
   // The rest don't invalidate analyses, they only trigger re-runs because we