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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Really like the approach now. Pretty minor code nits below only. =D



================
Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:856-858
+        auto *ResultFAMCP =
+            &CGAM.getResult<FunctionAnalysisManagerCGSCCProxy>(*C, CG);
+        ResultFAMCP->updateFAM(FAM);
----------------
Check that it doesn't hit an assert failure, but I think you can remove this one.


================
Comment at: llvm/include/llvm/IR/PassManager.h:812
+    auto *RetRes = &static_cast<ResultModelT *>(ResultConcept)->Result;
+    if (calledFromProxy) {
+      PreservedAnalyses PA = PreservedAnalyses::none();
----------------
asbirlea wrote:
> 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?
Ah, good point.

How about making this a `verifyNotInvalidated` method separate from `getCachedResult`? That still seems a bit cleaner to me than a bool parameter. What do you think?


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:249-251
+  // Create a new empty Result. This needs to have the FunctionAnalysisManager
+  // inside through the updateFAM() API whenever the FAM's module proxy goes
   // away.
----------------
Don't fully understand this comments wording... Maybe something more along the lines of:

> We just return an empty result. The caller will use the `updateFAM` interface to correctly register the relevant `FunctionAnalysisManager` based on the context in which this proxy is run.


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:341
+                                         FunctionAnalysisManager &FAM) {
+  auto *ResultFAMCP = &AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C, G);
+  ResultFAMCP->updateFAM(FAM);
----------------
Omit the variable?


================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:406-411
   bool NeedFAMProxy =
       AM.getCachedResult<FunctionAnalysisManagerCGSCCProxy>(*OldC) != nullptr;
+  FunctionAnalysisManager *FAM = nullptr;
+  if (NeedFAMProxy)
+    FAM =
+        &AM.getResult<FunctionAnalysisManagerCGSCCProxy>(*OldC, G).getManager();
----------------
Can this be simplified to:

```
FunctionAnalysisManager *FAM = nullptr;
if (auto *FAMProxy = AM.getCachedResult<FunctionAnalysisManagerCGSCCProxy(*OldC))
  FAM = &FAMProxy->getManager();
```

And then use `FAM` being non-null below instead of the `NeedFAMProxy` bool?



================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:705
+      if (HasFunctionAnalysisProxy) {
+        auto *ResultFAMCP =
+            &AM.getResult<FunctionAnalysisManagerCGSCCProxy>(*C, G);
----------------
Could omit the variable?


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1172
     // re-use the exact same logic for updating the call graph to reflect the
     // change.
+    // Inside the update, we also update the FunctionAnalysisManager in the
----------------
Add a blank line between the paragraphs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72893