[PATCH] D27198: [PM] Introduce the facilities for registering cross-IR-unit dependencies that require deferred invalidation.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Responses to more comments below, thanks for the review!



================
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.
----------------
jlebar wrote:
> s/, that/
I'm so bad at commas... I hope this is better now...


================
Comment at: include/llvm/IR/PassManager.h:506
 
+    bool invalidate(AnalysisKey *ID, IRUnitT &IR, const PreservedAnalyses &PA) {
+      SmallDenseMap<AnalysisKey *, bool, 8>::iterator IMapI;
----------------
jlebar wrote:
> ```
> /// Type-erased version of templated \c invalidate above.
> ```
> 
> ?
> 
> Also, real bummer we have to copy-paste this.
Done and factored into a common routine. Came up with a nice way to have a single implementation that is fast when it can be fast but generic/type-erased when it needs to be.


================
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
----------------
jlebar wrote:
> It's not clear what this is contrasting with (without the diff available :).
Reworded to hopefully make this more clear.


================
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) ||
----------------
jlebar wrote:
> 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<...>()?
I mean, I don't disagree with any of this, but I've not come up with a better alternative really.

I know there are going to be more sets than IR-unit derived ones such as CFG-preserving. =/ So bundling it inside the proxy doesn't seem like it'd be a great alternative... And it would still be quite hard to make work.

The key thing that needs to happen is that one layer needs to be able to introduce a preserved set for an IR unit, and then some other part of the code needs to subtract one analysis from that set, and then when we call 'invalidate' on *that* analysis it needs to not pay attention to the set.

Anyways, any better API ideas here are very, very welcome. =/


================
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:
> jlebar wrote:
> > Run-on sentence
> proxies
Yea, this is just a mess. Tried to improve, but complain more if it is still just not coming across well.


================
Comment at: lib/IR/PassManager.cpp:86
+      InnerAM->invalidate(F, PA);
+  }
 
----------------
jlebar wrote:
> ...wait, didn't I just read this function inCGSCCPassManager.cpp?  :(
> 
> Probably not something to be fixed in this patch.
You read a remarkably similar but subtly different function. =[ I'm not thrilled with this either, but factoring the code may be noisier than the duplication.


================
Comment at: unittests/IR/PassManagerTest.cpp:431
 
+/// A test analysis pass which chaches in its result the result from the above
+/// indirect analysis pass.
----------------
jlebar wrote:
> 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.
I don't actually think of it this way.

I think there is a common underlying idea of a pass, and there are two primary special cases: analysis passes and transformation passes.

I understand that many (most?) analysis passes tend to be trivial and we instead focus on the analysis result and caching it, but I don't want to neglect the fact that it is a pass that gets run over the IR. In that sense, the `AnalysisManager` *is* a `pass manager as well.

Anyways, I'm happy to spend some time debating this long term, but I'm not sure it's the right focus of this code review....


https://reviews.llvm.org/D27198