[DomTree] findNearestCommonDominator: assert the nodes are in tree

[MaskRay]

Summary

i.e. they cannot be unreachable from the entry (which usually indicate usage errors).

[kuhar]

Does this break any existing code?

llvm/include/llvm/Support/GenericDomTree.h

483

Maybe assert each node separately and assertion message explaining what this means?

483–493

I both NodeA and NodeB have to be non-null before the loop, and we unconditionally dereference NodeA at the return statement, then one of the three must be off: (1) the assertion, (2) the loop condition, (3) the return.

[MaskRay]

Delete an unneeded loop condition

[MaskRay]

llvm/include/llvm/Support/GenericDomTree.h

483–493

Good catch. I have deleted the redundant check.

check-llvm check-clang passed.

[kuhar]

One remaining thing: the function documentation needs to be updated:

If there is no such block then return nullptr.

This is obsolete, and it may be worth mentioning that A and B must have tree nodes.

Or actually, would it make sense to just make the function accept tree nodes instead of basic blocks?

[MaskRay]

Update comment

[MaskRay]

In D88758#2310914, @kuhar wrote:

One remaining thing: the function documentation needs to be updated:

If there is no such block then return nullptr.

This is obsolete, and it may be worth mentioning that A and B must have tree nodes.

Fixed.

Or actually, would it make sense to just make the function accept tree nodes instead of basic blocks?

There are 21 references of the function. Many don't have tree nodes around. Changing to tree nodes can inconvenience many call sites.

[kuhar]

OK, thanks for looking into this.

llvm/include/llvm/Support/GenericDomTree.h

NodeT *findNearestCommonDominator(NodeT *A, NodeT *B) const {
     if (!isPostDominator()) {
       NodeT &Entry = A->getParent()->front();
       if (A == &Entry || B == &Entry)
         return &Entry;
     }
 
     DomTreeNodeBase<NodeT> *NodeA = getNode(A);
     DomTreeNodeBase<NodeT> *NodeB = getNode(B);
-
-    if (!NodeA || !NodeB) return nullptr;
+    assert(NodeA && NodeB);
 
     // Use level information to go up the tree until the levels match. Then
     // continue going up til we arrive at the same node.
     while (NodeA && NodeA != NodeB) {
       if (NodeA->getLevel() < NodeB->getLevel()) std::swap(NodeA, NodeB);
 
       NodeA = NodeA->IDom;
     }
 
-    return NodeA ? NodeA->getBlock() : nullptr;
+    return NodeA->getBlock();
   }
 
   const NodeT *findNearestCommonDominator(const NodeT *A,
                                           const NodeT *B) const {
     // Cast away the const qualifiers here. This is ok since
     // const is re-introduced on the return type.
     return findNearestCommonDominator(const_cast<NodeT *>(A),
                                       const_cast<NodeT *>(B));