[PATCH] D27197: [PM] Support invalidation of inner analysis managers from a pass over the outer IR unit.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Updated with fixes to hopefully everything (please let me know if I missed something). Responses in a few cases below.



================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:179
+  /// explicit specialization of this method to handle that particular pair
+  /// of IR unit and inner CGSCCAnalysisManager.
+  bool invalidate(Module &M, const PreservedAnalyses &PA,
----------------
jlebar wrote:
> I don't understand this last sentence.  This is already a template specialization, so "this template" is...the template we're specializing?  Does each specialization of InnerAnalysisManagerProxy::Result::invalidate need to specialize as described here?
Sorry, I just kept a bunch of pointless documentation from the original template. Nuked this.


================
Comment at: include/llvm/IR/PassManager.h:770
 
-    /// \brief Handler for invalidation of the module.
+    /// \brief Handler for invalidation of the containing IR unit.
     ///
----------------
jlebar wrote:
> FWIW I don't think that "containing IR unit" is particularly helpful.
> 
> In my patch that rewrites these comments, I changed it to talk about the "larger" and "smaller" IR units:
> 
> ```
> /// \brief An analysis over a "larger" IR unit that provides access to an analysis manager over a "smaller" IR unit.
> class InnerAnalysisManagerProxy {
>   [...]
>   /// \brief Handler for invalidation of the "larger" IR unit, \c IRUnitT.
>   bool invalidate(...) {}
> }
> ```
> 
> But we can go over this separately.
Sure, happy to try different sets of terminology and see what works best.

I can see why "containing" would be problematic. The code itself currently uses "inner" and "outer" which at least seem somewhat better and I've made the comments match that for now.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:111
+  // Ok, we have a graph so we can propagate the invalidation down into it.
+  // Note that we do this even if the call graph is about to become invalid so
+  // that we trigger any secondary invalidation within the cached results for
----------------
jlebar wrote:
> Do we in fact do this if the call graph is about to become invalid?  The comment on the first if statement in this functions ays we go into the `if` if "the call graph is going to be invalidated".  Maybe this whole comment is stale?
This comment is indeed stale.


================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:127
+  template <typename T>
+  LambdaModulePass(T &&Arg) : Func(std::forward<T>(Arg)) {}
+  // We have to explicitly define all the special member functions because MSVC
----------------
jlebar wrote:
> Any reason not to take an std::function as the argument?
It seems more idiomatic. In other, more interesting cases it can actually matter because we run out of implicit conversions. It also avoids spelling out the function type twice.


================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:645
+                                      "  ret void\n"
+                                      "}\n");
+  {
----------------
jlebar wrote:
> Raw string?
Would it be an improvement? I'm not sure...

Anyways, should be a separate patch and applied to all the textual modules here if we can rely on this feature and it is worth using.


https://reviews.llvm.org/D27197