[PATCH] D58349: [Dominators] Fix and optimize edge insertion of depth-based search

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

@MaskRay, awesome, thanks for looking into this.

Does it affect the number of visited nodes and number of calls to `VisitInsertion`?

> depth(nca(x,y))+1 < depth(v) && there exists a path P from y to v where every w on P satisfies depth(v) <= depth(w)



> This reduces to a widest path problem (maximizing the weight of the
>  minimum vertex in the path) which can be solved by a modified version of
>  BFS with a bucket queue (Depth-based search as mentioned in the paper).

It would be great to also mention that in the implementation -- from what I remember, the paper doesn't mention the widest path problem.

Additionally, this change should be tested extensively -- domtree bugs tend to often be very difficult to discover.

If you are having fun playing with the incremental updater, I think there's much more to squeeze out of deletions -- the implementation cut corners in many cases, while deletions are known to be a bottleneck on some bitcode in JumpThreading and other loop transforms.



================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:746
     InsertionInfo II;
+    SmallVector<TreeNodePtr, 8> Stack;
     LLVM_DEBUG(dbgs() << "Marking " << BlockNamePrinter(To)
----------------
Is it possible to give `Stack` a less generic name, or comment on what it corresponds to in the paper?


================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:783
 
-    SmallPtrSet<TreeNodePtr, 8> Processed;
-
-    do {
-      TreeNodePtr Next = Stack.pop_back_val();
-      LLVM_DEBUG(dbgs() << " Next: " << BlockNamePrinter(Next) << "\n");
+    while (1) {
+      LLVM_DEBUG(dbgs() << " Next: " << BlockNamePrinter(TN) << "\n");
----------------
nit: while (true)


================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:812
+        break;
+      TN = Stack.pop_back_val();
+    }
----------------
I'd prefer not to reassign(scalar) function arguments, mostly for readability and debugability.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58349/new/

https://reviews.llvm.org/D58349