[PATCH] D72893: [NewPassManager] Add assertions when getting statefull cached analysis.
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 21 01:00:33 PST 2020
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
More information about the llvm-commits
mailing list