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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
