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

Justin Lebar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 6 18:06:39 PST 2016


jlebar added inline comments.


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:168
+  /// thus we don't need to invalidate *all* cached data associated with any
+  /// \c Module* in the \c CGSCCAnalysisManager.
+  ///
----------------
Perhaps we could be more concrete:

If the proxy analysis itself is preserved, then we assume that the set of SCCs in the Module hasn't changed.  Thus any pointers to SCCs in the CGSCCAnalysisManager are still valid, and we don't need to call \c clear on the CGSCCAnalysisManager.


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:173
+  /// the relevant inner set of their IR units) based on the set of preserved
+  /// analyses.
+  ///
----------------
What's an "inner set"?  (Maybe this whole parenthetical can go.)


================
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,
----------------
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?


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:189
+/// Provide a specialized run method for the \c CGSCCAnalysisManagerModuleProxy
+/// so it can pass the lazy call garph to the result.
+template <>
----------------
garph


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:447
+///
+/// Note that this does not mirror the proxy from a \c FunctionAnalysisManager
+/// to a \c Module, and instead indirects through that proxy.
----------------
Suggest rephrasing "does not mirror the proxy" -- it's hard to understand what you mean.

Perhaps just remove this para.


================
Comment at: include/llvm/Analysis/CGSCCPassManager.h:456
+/// delegate module-wide invalidation to the direct \c FunctionAnalysisManager
+/// proxy at that layer.
+class FunctionAnalysisManagerCGSCCProxy
----------------
Starting from "Instead" is very hard to parse.  Perhaps something like:

> Instead, this layer is only responsible for SCC-local invalidation events.  We work with the module's FunctionAnalysisManager to invalidate function analyses.


================
Comment at: include/llvm/IR/PassManager.h:770
 
-    /// \brief Handler for invalidation of the module.
+    /// \brief Handler for invalidation of the containing IR unit.
     ///
----------------
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.


================
Comment at: include/llvm/IR/PassManager.h:786
+    /// explicit specialization of this method to handle that particular pair
+    /// of IR unit and inner AnalysisManagerT.
     bool invalidate(
----------------
This comment makes sense -- maybe we simply don't need the one I commented on earlier that didn't make sense.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:94
+  //
+  // We also directly invalidate the FAM's module proxy if necessary and if
+  // that proxy isn't preserved we can't preserve this proxy either. We rely on
----------------
Comma before "and".


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:97
+  // it to handle module -> function analysis invalidation in the face of
+  // structural changes and so if unavailable we conservatively clear the
+  // entire SCC layer as well rather than trying to do invaliadtion ourselves.
----------------
"if it's unavailable"


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:105
+    // And the proxy itself should be marked as invalid so that we can observe
+    // the new call graph if and when created. This isn't strictly necessary
+    // because we cheat above, but is still useful.
----------------
"when it's created", although maybe it's good enough to say "we can observe the new call graph."


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:110
+
+  // 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
----------------
comma before "so".


================
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
----------------
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?


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:121
+
+  // Otherwise false to indicate that this result is still a valid proxy.
+  return false;
----------------
Suggest s/Otherwise/Return/ because the thing we're contrasting against is somewhat far away.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:147
+  // This allows us to rely on the FunctionAnalysisMangaerModuleProxy to
+  // handle non-SCC parts of the invalidation of function analyses.
+  auto &MAM = AM.getResult<ModuleAnalysisManagerCGSCCProxy>(C, CG).getManager();
----------------
Would it be correct to rephrase this to say

> rely on the FAMMP to invalidate the function analyses.

?  If so, maybe we can say that?  If not, I do not understand what this is trying to say.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:155
+
+  // Note that we hard-code invalidation handling of this proxy in the CGSCC
+  // analysis manager's Module proxy. This avoids the need to do anything
----------------
s/hard-code/special-case/?


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:171
+  // module-level FAM proxy becomes invalid the entire SCC layer is cleared
+  // which will include this proxy.
+  return false;
----------------
Suggest

> entire SCC layer, which includes this proxy, is cleared.


================
Comment at: lib/Analysis/LoopPassManager.cpp:33
+  // If this proxy isn't marked as preserved, then we can't even invalidate
+  // individual function analyses, there may be an invalid set of Function
+  // objects in the cache making it impossible to incrementally preserve
----------------
 * s/, there/, as there/
 * s/making/, making/

But maybe even better would be to rephrase a bit:

> If this proxy isn't marked as preserved, the set of Function objects in the module may have changed.  We therefore can't call InnerAM->invalidate(), because any pointers to Functions it has may be stale.  Instead, we call clear().


================
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
----------------
Any reason not to take an std::function as the argument?


================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:129
+  // We have to explicitly define all the special member functions because MSVC
+  // refuses to generate them.
+  LambdaModulePass(LambdaModulePass &&Arg) : Func(std::move(Arg.Func)) {}
----------------
Not anymore!


================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:555
+  }
+  {
+    // Next module pass checks that marking the SCC analysis preserved is
----------------
Any reason not to write these as independent TESTs?  As-is I have to read carefully to see that you reset the SCCAnalysisRuns counter between tests, and it's not obvious whether you're (intentionally or unintentionally) carrying any additional state between the various blocks.


================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:636
+
+  // Create a very simple module with a single function & SCC to make testing
+  // these issues much easier.
----------------
s/&/and/ because don't we already have enough meanings for "&" in C++?  :)


================
Comment at: unittests/Analysis/CGSCCPassManagerTest.cpp:645
+                                      "  ret void\n"
+                                      "}\n");
+  {
----------------
Raw string?


https://reviews.llvm.org/D27197





More information about the llvm-commits mailing list