[PATCH] D85109: [NewPM] Clear abandoned analyses set when preserveSet is called

[Yuanfang Chen via Phabricator via llvm-commits]

ychen created this revision.
ychen added reviewers: asbirlea, chandlerc, aeubanks.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
ychen requested review of this revision.

Fix the extra invalidation issue found in D84959 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85109

Files:
  llvm/include/llvm/IR/PassManager.h
  llvm/unittests/IR/PassManagerTest.cpp


Index: llvm/unittests/IR/PassManagerTest.cpp
===================================================================
--- llvm/unittests/IR/PassManagerTest.cpp
+++ llvm/unittests/IR/PassManagerTest.cpp
@@ -396,10 +396,19 @@
   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>()
Index: llvm/include/llvm/IR/PassManager.h
===================================================================
--- llvm/include/llvm/IR/PassManager.h
+++ llvm/include/llvm/IR/PassManager.h
@@ -186,9 +186,22 @@
       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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D85109.282513.patch
Type: text/x-patch
Size: 2428 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200803/d4e2206c/attachment.bin>