[PATCH] D41298: [PDT] Add verifyDomTree and verifyAnalysis for Post Dom Trees

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: include/llvm/Analysis/PostDominators.h:40
+  /// verification fails.
+  void verifyDomTree() const;
 };
----------------
kuhar wrote:
> I don't think it's a good idea to expose it externally. For debugging, you can just do `assert(PDT.verify())`.
> The problem I see is that people will call it not realizing that it doesn't perform full verification and does not check each property individually, resulting in worse errors than plain `PDT.verify()`.
> 
> I think we should just keep it internal and only use in verifyAnalysis, or just inline it there.
Interesting. I think there is value in keeping dom/postdom trees equivalent, whatever we end up doing.

My understanding is that there are two different usecases for checking the dom tree. There's the case for people like yourself who are writing the domtree construction code. This need good quality checks that are hard to do, and that check various properties of the tree are correct.

Then there's guys who are just trying to use the domtree updater to preserve the tree across passes. Here you can almost always presume that a newly calculated tree is correct. In my experience, the best error message says "The tree should look like -this-, but you made it look like -this!-". Messages like "node %x is not reachable when it's sibling %y is removed", whilst I can see the value in being more succinct (and better tests of the tree), they are often not as useful as being able to eyeball the difference between two trees and notice that y is in the wrong place.

So this may just be a difference in what we use these for. And may just be my two cents :) I found the error messages coming out of verifyDomTree often quite useful. That said, I have not used it it any code anywhere, just whilst debugging. And like you say, perhaps assert(DT.verify(Fast)) is a better option for this (the CFG output can be a bit noisy :) )

So having said all that.. (comment below)


================
Comment at: include/llvm/IR/Dominators.h:180
   /// This should only be used for debugging as it aborts the program if the
   /// verification fails.
   void verifyDomTree() const;
----------------
kuhar wrote:
> Here, I think we should change the docs to say that this is a legacy debugging function, and point users to `DT.verify` instead. When we first introduced the new verifiers,  I completely replaced `verifyDomTree` with `verify`, but then realized that people use it in regular passes in debug builds and expect it to be fast.
> 
> With the new verification levels, I think it would be better to either completely replace all uses with `DT.verify(Fast)`, or just keep it as a wrapper around `.verify()` that only prints CFG upon failure and asserts.
> (This doesn't mean it propose to do all of it in one patch.)
Yeah, I think this sounds best. Removing verifyDomTree entirely and just using verify everywhere we need it. I can probably do that pretty quickly, it's just a fairly mechanical change of replacing one function call with another.


https://reviews.llvm.org/D41298