[PATCH] D27190: Track validity of pass results

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Generally I like where this is going, especially as it gives us a nice assert for getAnalysis that would have cought several bugs for me.

However, I'd really like to isolate this for analysis passes only so that we don't have to do so much with skipFunction. Is there no way to do that? Relatedly, the logic for when to set things available doesn't really make sense in the current patch. Might just need comments, but it seems very non-obvious when things become available.



================
Comment at: include/llvm/Pass.h:85
   PassKind Kind;
+  bool Available;   ///< True if the pass result is available
+
----------------
I wouldn't put the doxygen comment here (or use end-of-line doxygen generally). Instead see my comment below...


================
Comment at: include/llvm/Pass.h:96
   PassKind getPassKind() const { return Kind; }
+  bool isAvailable() const { return Available; }
+
----------------
This needs a doxygen comment as much or more than the rest. =] I also wouldn't try to group with the above method.


================
Comment at: include/llvm/Pass.h:98-101
+  /// Sets a flag to indicates that the pass has a valid result.
+  /// A pass result can be invalidated if the pass was skipped or freed.
+  virtual void markAvailable(bool x = true) { Available = x; }
 
----------------
Does this need to be virtual? It seems a waste just for immutable passes. Is it even necessary there? It seems like those should be handled by the existing logic...

Also, I would find a 'setAvailable' without a default argument more clear. Or two functions, one to mark and one to clear.


================
Comment at: lib/Analysis/LoopInfo.cpp:730-731
   if (VerifyLoopInfo) {
-    auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-    LI.verify(DT);
+    if (auto *Analysis = getAnalysisIfAvailable<DominatorTreeWrapperPass>()) {
+      auto &DT = Analysis->getDomTree();
+      LI.verify(DT);
----------------
This looks like it is working around a bug. We should always have domtree when we are running LoopInfo...


================
Comment at: lib/Analysis/LoopPass.cpp:178
       LoopPass *P = getContainedPass(Index);
+      P->markAvailable();
       Changed |= P->doInitialization(L, *this);
----------------
I don't understand why passes are being marked available here?


https://reviews.llvm.org/D27190