[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

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.

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.
+  // AAResults preserves the AAManager by default, due to the stateless nature
+  // of AliasAnalysis. Do not invalidate it here.
 
   // Check all of the dependencies registered.
   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;

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

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

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