[PATCH] D27198: [PM] Introduce the facilities for registering cross-IR-unit dependencies that require deferred invalidation.
Justin Lebar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 6 18:44:33 PST 2016
jlebar added inline comments.
================
Comment at: include/llvm/IR/PassManager.h:75
/// the IR is not mutated at all.
class PreservedAnalyses {
public:
----------------
The changes here seem like a type-safety nightmare.
Could we require that these abstract sets inherit from some type? That would also help with the explanation, I think, by making "abstract sets that might be preserved" a concrete thing, namely types that inherit from FooType.
================
Comment at: include/llvm/IR/PassManager.h:100
+ // If not covered by the "all" set, add this to the explicitly preserved
+ // set.
if (!areAllPreserved())
----------------
s/If not covered by the "all" set/If we're not already preserving all analyses (other than those in NotPreservedAnalysisIDs)/
(Problem is there's no direct object for "covered".)
================
Comment at: include/llvm/IR/PassManager.h:106
+ /// Mark a particular pass as abandoned, removing it from the preserved set
+ /// even if covered by some other set.
+ template <typename PassT> void abandon() { abandon(PassT::ID()); }
----------------
and even if already explicitly marked as preserved.
================
Comment at: include/llvm/IR/PassManager.h:113
+ /// Note that you can only abandon a specific analysis, not a *set* of
+ /// analyses.
+ void abandon(AnalysisKey *ID) {
----------------
Should this note appear above as well?
================
Comment at: include/llvm/IR/PassManager.h:119
+ // And add it to the explicitly not-preserved set so that even if there is
+ // some general set being preserved that won't cause this particular
+ // analysis to be preserved.
----------------
s/, that/
================
Comment at: include/llvm/IR/PassManager.h:136
+ // The intersection requires the union of the explicitly *not* preserved
+ // IDs and the intersection of the preserved IDs.
+ for (auto ID : Arg.NotPreservedAnalysisIDs) {
----------------
Nit, I would emphasize "*union*" and "*intersection*", rather than "*not*".
================
Comment at: include/llvm/IR/PassManager.h:158
+ // The intersection requires the union of the explicitly *not* preserved
+ // IDs and the intersection of the preserved IDs.
+ for (auto ID : Arg.NotPreservedAnalysisIDs) {
----------------
ibid.
================
Comment at: include/llvm/IR/PassManager.h:179
+ /// Query whether an abstract pass ID is marked as preserved by this
/// set.
+ bool preserved(AnalysisKey *ID,
----------------
Looks like this comment should be updated too?
================
Comment at: include/llvm/IR/PassManager.h:235
+ /// analysis not preserved. It always wins over the above set and should not
+ /// include the "all" set as that would be better done by clearing both sets
+ /// to be empty.
----------------
s/ as/, as/
================
Comment at: include/llvm/IR/PassManager.h:236
+ /// include the "all" set as that would be better done by clearing both sets
+ /// to be empty.
+ SmallPtrSet<AnalysisKey *, 2> NotPreservedAnalysisIDs;
----------------
s/to be empty//
================
Comment at: include/llvm/IR/PassManager.h:236
+ /// include the "all" set as that would be better done by clearing both sets
+ /// to be empty.
+ SmallPtrSet<AnalysisKey *, 2> NotPreservedAnalysisIDs;
----------------
jlebar wrote:
> s/to be empty//
Actually, it's stronger than "should never contain the 'all' set" -- it should never contain *any* abstract sets of analyses.
================
Comment at: include/llvm/IR/PassManager.h:506
+ bool invalidate(AnalysisKey *ID, IRUnitT &IR, const PreservedAnalyses &PA) {
+ SmallDenseMap<AnalysisKey *, bool, 8>::iterator IMapI;
----------------
```
/// Type-erased version of templated \c invalidate above.
```
?
Also, real bummer we have to copy-paste this.
================
Comment at: include/llvm/IR/PassManager.h:675
void invalidate(IRUnitT &IR, const PreservedAnalyses &PA) {
- // Short circuit for common cases of all analyses being preserved.
- if (PA.areAllPreserved() || PA.preserved<AllAnalysesOn<IRUnitT>>())
+ // We can only use one short circuit because even if another set is
+ // preserved there might be abandoned analyses within that set which still
----------------
It's not clear what this is contrasting with (without the diff available :).
================
Comment at: include/llvm/IR/PassManager.h:956
+/// Because outer analyses aren't invalidated while these IR units are being
+/// precessed, we have to register and handle these as deffered invalidation
+/// events.
----------------
deffered
================
Comment at: include/llvm/IR/PassManager.h:988
+ // this entire system should be changed to some other deterministic
+ // datastructure such as a setvector of a pair of pointers.
+ auto InvalidatedIt = std::find(InvalidatedIDList.begin(),
----------------
s/datastructure/data structure/
================
Comment at: include/llvm/IR/PassManager.h:1007
+ /// which need to be invalidated.
+ DenseMap<AnalysisKey *, TinyPtrVector<AnalysisKey *>>
+ OuterAnalysisInvalidationMap;
----------------
You don't want a version with inline storage?
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:103
// entire SCC layer as well rather than trying to do invaliadtion ourselves.
- if (!PA.preserved<CGSCCAnalysisManagerModuleProxy>() ||
+ if (!PA.preserved<CGSCCAnalysisManagerModuleProxy, AllAnalysesOn<Module>>() ||
Inv.invalidate<LazyCallGraphAnalysis>(M, PA) ||
----------------
Hm, now that I see it being used, I am even less thrilled about this API. It's not at all obvious what the second template argument means here, and it also seems super easy to forget to pass this the relevant arguments. In addition, if I ever add a new abstract set, I have to go and modify every preserved() call.
Would it be out of the question to encapsulate within (say) the CGSCCAnalysisManagerModuleProxy type the sets that cover it, so that we could continue to pass only one type to preserved<...>()?
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:114
+ // Directly check if the relevant set is preserved.
+ bool AreSCCAnalysesPreserved =
----------------
Not sure this comment is helpful, although maybe some foreshadowing about what we're going to do with this information might help.
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:130
+ // Check to see whether the preserved set needs to be pruned based on
+ // module-level analysis invalidation that triggers deffered invalidation
+ // registered with the outer analysis manager proxy for this SCC.
----------------
deffered
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:131
+ // module-level analysis invalidation that triggers deffered invalidation
+ // registered with the outer analysis manager proxy for this SCC.
+ if (auto *OuterProxy =
----------------
Run-on sentence
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:131
+ // module-level analysis invalidation that triggers deffered invalidation
+ // registered with the outer analysis manager proxy for this SCC.
+ if (auto *OuterProxy =
----------------
jlebar wrote:
> Run-on sentence
proxies
================
Comment at: lib/Analysis/CGSCCPassManager.cpp:146
+
+ // Check if we needed a custom PA set, and if so we'll need to run the
+ // inner invalidation.
----------------
Split into two sentences.
================
Comment at: lib/IR/PassManager.cpp:86
+ InnerAM->invalidate(F, PA);
+ }
----------------
...wait, didn't I just read this function inCGSCCPassManager.cpp? :(
Probably not something to be fixed in this patch.
================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:846
+/// FIXME: Currently this doesn't also depend on a function analysis and if it
+/// did we would fail to invalidated it correctly.
+struct TestIndirectSCCAnalysis
----------------
invalidate
================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:894
+
+/// A test analysis pass which chaches in its result the result from the above
+/// indirect analysis pass.
----------------
chaches
================
Comment at: unittests/IR/PassManagerTest.cpp:431
+/// A test analysis pass which chaches in its result the result from the above
+/// indirect analysis pass.
----------------
silvas wrote:
> nit: avoid the terminology "analysis pass". In the new PM analyses and transformations are separate concepts. The term "pass" doesn't help because it conflates the two (and many uses in the code use "pass" to really mean "transformation", so "analysis pass" is particularly confusing and old-PM'ish).
>
> Hopefully some day we can rename things to be more consistent. Really the new "PM" has just two main things: an `AnalysisCache` class and a bunch of composable `TransformationRunner`'s. There isn't a conflated concept of "pass" (which can be either a transformation or an analysis) like in the old PM.
+1 to that in principle, although if we actually carry that out, we're going to have to do a big refactoring, so until then my personal preference would be that we should just say whatever is clear, rather than adding in the new terminology in places where "pass" would, in the current state, be more clear.
Not begging the specific question of what to say here.
https://reviews.llvm.org/D27198
More information about the llvm-commits
mailing list