[PATCH] D72893: [NewPassManager] Add assertions when getting statefull cached analysis.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/IR/PassManager.h:812
+    auto *RetRes = &static_cast<ResultModelT *>(ResultConcept)->Result;
+    if (calledFromProxy) {
+      PreservedAnalyses PA = PreservedAnalyses::none();
----------------
Could this logic be moved into the callers that pass true here to avoid the need for a book parameter?


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:319
+        // ```
+        // For the purposes of this unittest, use the above MAM directly.
         if (TestModuleAnalysis::Result *TMA =
----------------
You added a book returning method for unittesting without triggering the assert, is that not enough here? It does seem much cleaner to me than the friend declaration - much harder to misuse.


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:394
+    const auto &MAMProxy = AM.getResult<ModuleAnalysisManagerCGSCCProxy>(C, CG);
+    bool TMA = MAMProxy.cachedResultExists<TestModuleAnalysis>(
+        *C.begin()->getFunction().getParent());
----------------
Just sink the test into the if?


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:413
+            AM.getResult<ModuleAnalysisManagerCGSCCProxy>(C, CG);
+        bool TMA = MAMProxy.cachedResultExists<TestModuleAnalysis>(
             *C.begin()->getFunction().getParent());
----------------
Same as above.


================
Comment at: llvm/unittests/Analysis/CGSCCPassManagerTest.cpp:903
+    //  *C.begin()->getFunction().getParent());
+    // Use MAM, for the purposes of this unittest.
     auto &MDep = *MAM.getCachedResult<TestModuleAnalysis>(
----------------
Here and below where similar patterns actually necessitate the friend function, I think the test is actually exercising specific functionality that your change makes unsupported, so these tests should be changed or removed.

I think they still are useful for the case where the outer analysis would survive a preserve-none invalidate, but *not* survive *forced* invalidation. In that case the cached results should be fine and safe (and your assert pass) but still require this test to register the outer analysis dependency to handle forced invalidation.

I think you can update this to use your new API making the cached result return false on a call to invalidate, but then forcing it's invalidation where necessary.

Some tests may become no-ops or redundant with that change and those should just be removed. Does that make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72893