[PATCH] D27190: Track validity of pass results

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 3 inline comments as done.
sepavloff added a comment.

> However, I'd really like to isolate this for analysis passes only so that we don't have to do so much with skipFunction.

This facility indeed is used only for analysis passes. However analysis passes share the same code path with non-analysis ones and it is easier to maintain the flag for all passes. Besides there is no simple way to know if this particular pass is an analysis. There is a comment to `PassInfo` that states it can be obtained from a `Pass` by `getPassInfo` (https://github.com/llvm-mirror/llvm/blob/master/include/llvm/PassInfo.h#L28-L29), however such method does not exist in `Pass`.

On the other hand the new flag is not tied to analysis only, actually it tracks the fact whether the pass was executed or not. I renamed the flag to `Executed` and associated function accordingly in hope that the code becomes clearer.

> 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.

When the flag must change is described in the comment to the method `setExecuted`.



================
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; }
 
----------------
chandlerc wrote:
> 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.
The method is no more virtual. It however required a check in `freePass` to avoid invalidation of immutable passes. There is no default argument now.


================
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);
----------------
chandlerc wrote:
> This looks like it is working around a bug. We should always have domtree when we are running LoopInfo...
This is not about running `LoopInfo`, it is about *verification* of it. This method are run by a pass that clams it preserves `LoopInfo`. The `LoopInfo` results are valid at this point, but `DomTree` may be already freed because the last user of it has already executed.

Generally we have at least three options here:
- skip verification if `DomTree` is not available,
- do not free unneeded passes if verification is required,
- make passes transitively dependent on passes required for verification.

Of these options the first is the most obvious and the least pervasive.


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


https://reviews.llvm.org/D27190