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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: include/llvm/Analysis/PostDominators.h:40
+  /// verification fails.
+  void verifyDomTree() const;
 };
----------------
dmgreen wrote:
> 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)
So instead, we could just add a fresh domtree printing upon error to verify(). I mean, this is what people usually do manually when they debug these things. The current error messages coming from verify() are usually quite concise, so I think it makes sense to just make it print a freshly constructed tree below in addition to that.


https://reviews.llvm.org/D41298