[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;
 };
----------------
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.


================
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;
----------------
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.)


https://reviews.llvm.org/D41298