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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 2 inline comments as done.
asbirlea added inline comments.


================
Comment at: llvm/include/llvm/IR/PassManager.h:812
+    auto *RetRes = &static_cast<ResultModelT *>(ResultConcept)->Result;
+    if (calledFromProxy) {
+      PreservedAnalyses PA = PreservedAnalyses::none();
----------------
chandlerc wrote:
> Could this logic be moved into the callers that pass true here to avoid the need for a book parameter?
I haven't addressed this. Yes, I can move the logic in to the caller but I need to make the AnalysisManagerT::Invalidator constructor public.
Is there another way to do this?


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:224-233
+  // This allows us to rely on the FunctionAnalysisManagerModuleProxy to
   // invalidate the function analyses.
-  auto &MAM = AM.getResult<ModuleAnalysisManagerCGSCCProxy>(C, CG).getManager();
+  auto &MAMProxy =
+      AM.getResult<ModuleAnalysisManagerCGSCCProxy>(C, CG).getInnerManager();
   Module &M = *C.begin()->getFunction().getParent();
-  auto *FAMProxy = MAM.getCachedResult<FunctionAnalysisManagerModuleProxy>(M);
+  auto *FAMProxy =
+      MAMProxy.getCachedResult<FunctionAnalysisManagerModuleProxy>(M);
----------------
asbirlea wrote:
> chandlerc wrote:
> > So, now that I actually read this code correctly, I understand better why this just doesn't fit anything.
> > 
> > The problem here is that we're not supposed to reconstruct module analyses that *must* be available at this layer. Those are supposed to be passed as parameters to the `run` method the way the `LazyCallGraph` is. Because then, an actual module pass (the CG adapter) will run them (as it can) and pass them along. But that's not a good fit *here* because every other CGSCC pass should use *this* proxy rather than one passed in...
> > 
> > The CGSCC layering is so complicated. I really wish I had any ideas how to really simplify things. Anyways.
> > 
> > 
> > I think there is one step that would be slightly cleaner than this. I would change this to just assert that the FAM proxy has been cached, and to construct an *empty* result object.
> > 
> > Then I would change the result object to allow the CGSCC walk (CGSCCPassManager.h:833) to actually get the result, and update it with the FAM. There, the code is actually a *module* pass, and so it can directly get the `FAMProxy` without issue. That will also resolve the FIXME on line 831 in that file as there will be a clear reason why we have to specially set up this proxy -- we need to pass something from the Module layer into the SCC layer that could otherwise be invalidated.
> I made the changes according to this suggestion. 
> Some notable changes:
> - The empty result needs updating whenever it was retrieved before to get it created, and after an invalidate event.
> - Updated `updateCGAndAnalysisManagerForFunctionPass` and `updateCGAndAnalysisManagerForCGSCCPass` APIs to need a FunctionaAnalysisManager. This affects Coroutines, CallGraphUpdater (needed by Attributor), Inliner and the unit tests in CGSCCPassManagerTest.
> - I left a comment at ~CGSCCPassManager.cpp:235. Having the assert to check that the FunctionAnalysisManagerModuleProxy exists will change the pass order in the tests, and it will behave differently with vs without asserts. So this would either be removed entirely (the tests are updated in this diff), or have the `cachedResultExists` run outside of the assert and `(void)` the result in release mode (the tests won't change then).
> 
> Please let me know what you think.
Re-added the assert and undid the test changes. This always gets the result for the proxy here, as before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72893