Analyses that are statefull should not be retrieved through a proxy from
an outer IR unit, as these analyses are only invalidated at the end of
the inner IR unit manager.
This patch disallows getting the outer manager and provides an API to
get a cached analysis through the proxy. If the analysis is not
stateless, the call to getCachedResult will assert.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: unknown.
clang-tidy: unknown.
clang-format: unknown.
Build artifacts: diff.json, console-log.txt
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
llvm/include/llvm/IR/PassManager.h | ||
---|---|---|
800 | Could this logic be moved into the callers that pass true here to avoid the need for a book parameter? | |
llvm/unittests/Analysis/CGSCCPassManagerTest.cpp | ||
321 | 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. | |
395 | Just sink the test into the if? | |
412 | Same as above. | |
893 | 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? |
llvm/include/llvm/IR/PassManager.h | ||
---|---|---|
1147 | A quick clarification before addressing all the comments: only FunctionAnalysisManagerCGSCCProxy is a friend and can get the actual manager by calling getInnerManager. The unit tests do not have access to this API. They are using managers already declared in the test itself and it's probably worth not allowing even that, because, you know, it's hack to allow something disallowed... |
llvm/lib/Analysis/CGSCCPassManager.cpp | ||
---|---|---|
235–241 | 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. |
llvm/lib/Analysis/CGSCCPassManager.cpp | ||
---|---|---|
235–241 | I made the changes according to this suggestion.
Please let me know what you think. |
Simplified the instances where the proxy result needs to set the FAM.
Added comments on where this is needed (when an SCC is first seen not after an invalidation).
llvm/include/llvm/IR/PassManager.h | ||
---|---|---|
800 | 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. | |
llvm/lib/Analysis/CGSCCPassManager.cpp | ||
235–241 | Re-added the assert and undid the test changes. This always gets the result for the proxy here, as before. |
Really like the approach now. Pretty minor code nits below only. =D
llvm/include/llvm/Analysis/CGSCCPassManager.h | ||
---|---|---|
844–850 | Check that it doesn't hit an assert failure, but I think you can remove this one. | |
llvm/include/llvm/IR/PassManager.h | ||
800 | 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? | |
llvm/lib/Analysis/CGSCCPassManager.cpp | ||
247–249 | Don't fully understand this comments wording... Maybe something more along the lines of:
| |
340 | Omit the variable? | |
403–408 | 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? | |
702 | Could omit the variable? | |
llvm/lib/Transforms/IPO/Inliner.cpp | ||
960–965 | Add a blank line between the paragraphs? |
llvm/include/llvm/Analysis/CGSCCPassManager.h | ||
---|---|---|
844–850 | Removing this hits the assertion. |
Check that it doesn't hit an assert failure, but I think you can remove this one.