[NewPM] Clear abandoned analyses set when preserveSet is called

[ychen]

Summary

Fix the extra invalidation issue found in D84959. The issue was exposed when
PreservedAnalyses::abandon was first used in a regular pass.

The extra invalidation was due to the module analysis manager
calling its invalidate with a PreservedAnalyses which contains the
abandoned LVI. The abandoned LVI is returned to outer pass manager
because PreservedAnalyses::intersect unions all the abandoned analyses.

We should not return abandoned analyses to outer pass manager because
the necessary invalidation has already happened in the current pass
manager, which indicated by things like
PA.preserveSet<AllAnalysesOn<IRUnitT>>().

preserveSet<CFGAnalyses> is an exception to this so I propose to
rename it to preserve<CFGAnalyses> in order to make the usage of
preserveSet very specific.

This association of abandon with preserveSet is intended. It does
get tricky because the order of calls to abandon and preserveSet
become relevant but IMHO this association is helpful for reasoning the
code.

[ychen]

• update test

[asbirlea]

llvm/include/llvm/IR/PassManager.h

192

I'd be fine with renaming this with this mouthful name. Not just because it's more informative, but it's more likely to remain internally used, or easy to catch if it's not. I don't see how we can assert/enforce this easily.

[asbirlea]

llvm/include/llvm/IR/PassManager.h

192

Actually this doesn't look quite right. It's clearing all analyses, for all IR units.
After talking with chandlerc@, a better approach would be clearing in invalidate. I'm looking into the alternative approach.

[ychen]

llvm/include/llvm/IR/PassManager.h

192

Agree. Doing it in invalidate works too. Any reason clearing all analyses, for all IR units is wrong?

The scenario I was thinking are

/// - (abandoned inner analyses) the invalidation has happened in inner IR
///   manager, no need to let the outer manager know and they should not care.
///   If abandoned inner analyses were returned, it may trigger another round
///   of inner analyses invalidation from outer manager through the proxy
///   (InnerAnalysisManagerProxy).
/// - (abandoned outer analyses) abandoning has the same effect of not
///   preserving it.

llvm/include/llvm/IR/PassManager.h

void preserve(AnalysisKey *ID) {
     NotPreservedAnalysisIDs.erase(ID);
 
     // If we're not already preserving all analyses (other than those in
     // NotPreservedAnalysisIDs).
     if (!areAllPreserved())
       PreservedIDs.insert(ID);
   }
 
-  /// Mark an analysis set as preserved.
+  /// Mark an analysis set as preserved. This is only called by inner pass
+  /// manager to tell outer pass manager that inner analyses are up-to-date.
+  /// Regular passes should not call this.
+  /// FIXME: rename this to `preserveSetAndClearAbandonedAnalyses`?
   template <typename AnalysisSetT> void preserveSet() {
     preserveSet(AnalysisSetT::ID());
+    NotPreservedAnalysisIDs.clear();
+  }
+
+  // TODO: Using `preserve<CFGAnalyses>` instead so we could add comment to
+  // preserveSet saying that it is restricted to usage introduced in D23691.
+  // `preserveSet<CFGAnalyses>` is semantically correct but
+  // `preserve<CFGAnalyses>` helps reason about the code which IMHO is more
+  // important.
+  template <> void preserveSet<CFGAnalyses>() {
+    preserveSet(CFGAnalyses::ID());
   }
 
   /// Mark an analysis set as preserved using its ID.
   void preserveSet(AnalysisSetKey *ID) {
     // If we're not already in the saturated 'all' state, add this set.
     if (!areAllPreserved())
       PreservedIDs.insert(ID);
   }

llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll

 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O3-NEXT: Running pass: ControlHeightReductionPass on foo
 ; CHECK-O3-NEXT: Running analysis: RegionInfoAnalysis on foo
 ; CHECK-O3-NEXT: Running analysis: DominanceFrontierAnalysis on foo
 ; CHECK-O-NEXT: Finished {{.*}}Function pass manager run.
 ; CHECK-O-NEXT: Finished CGSCC pass manager run.
 ; CHECK-O-NEXT: Finished {{.*}}Module pass manager run.
 ; CHECK-O-NEXT: Finished {{.*}}Module pass manager run.
-; CHECK-O23SZ-NEXT: Clearing all analysis results for: <possibly invalidated loop>
-; CHECK-O23SZ-NEXT: Invalidating analysis: DominatorTreeAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: MemorySSAAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: LoopAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: PostDominatorTreeAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: BranchProbabilityAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: BlockFrequencyAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: ScalarEvolutionAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: InnerAnalysisManagerProxy
-; CHECK-O23SZ-NEXT: Invalidating analysis: PhiValuesAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: MemoryDependenceAnalysis
-; CHECK-O23SZ-NEXT: Invalidating analysis: DemandedBitsAnalysis
-; CHECK-O3-NEXT: Invalidating analysis: DominanceFrontierAnalysis
-; CHECK-O3-NEXT: Invalidating analysis: RegionInfoAnalysis
-; CHECK-O23SZ-NEXT: Clearing all analysis results for: foo
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis on bar
 ; CHECK-EXT: Running pass: {{.*}}::Bye
 ; CHECK-O-NEXT: Finished {{.*}}Module pass manager run.
-; CHECK-O23SZ-NEXT: Clearing all analysis results for: foo
 ; CHECK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-O-NEXT: Running pass: PrintModulePass
 
 ; Make sure we get the IR back out without changes when we print the module.
 ; CHECK-O-LABEL: define void @foo(i32 %n) local_unnamed_addr {
 ; CHECK-O-NEXT: entry:
 ; CHECK-O-NEXT:   br label %loop
 ; CHECK-O:      loop:

llvm/unittests/IR/PassManagerTest.cpp

TEST(PreservedAnalysisTest, Abandon) {
   EXPECT_FALSE(PA.getChecker<TestFunctionAnalysis>().preserved());
 
   // Repeated is fine, and abandoning if they were never preserved is fine.
   PA.abandon<TestFunctionAnalysis>();
   EXPECT_FALSE(PA.getChecker<TestFunctionAnalysis>().preserved());
   PA.abandon<TestModuleAnalysis>();
   EXPECT_FALSE(PA.getChecker<TestModuleAnalysis>().preserved());
 
-  // Even if the sets are preserved, the abandoned analyses' checker won't
-  // return true for those sets.
+  // If the sets are preserved after the analyses are abandoned, the abandoned
+  // analyses' checker will return true for those sets.
   PA.preserveSet<AllAnalysesOn<Function>>();
   PA.preserveSet<AllAnalysesOn<Module>>();
+  EXPECT_TRUE(PA.getChecker<TestFunctionAnalysis>()
+                  .preservedSet<AllAnalysesOn<Function>>());
+  EXPECT_TRUE(PA.getChecker<TestModuleAnalysis>()
+                  .preservedSet<AllAnalysesOn<Module>>());
+
+  // If the sets are preserved before the analyses are abandoned, the abandoned
+  // analyses' checker will return false for those sets.
+  PA.abandon<TestFunctionAnalysis>();
+  PA.abandon<TestModuleAnalysis>();
   EXPECT_FALSE(PA.getChecker<TestFunctionAnalysis>()
                    .preservedSet<AllAnalysesOn<Function>>());
   EXPECT_FALSE(PA.getChecker<TestModuleAnalysis>()
                    .preservedSet<AllAnalysesOn<Module>>());
 
   // But an arbitrary (opaque) analysis will still observe the sets as
   // preserved. This also checks that we can use an explicit ID rather than
   // a type.