[PATCH] D75507: [mlir] Extended Dominance analysis with a function to find the nearest common dominator of two given blocks.

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

Thanks for the patch! I added a few comments.



================
Comment at: mlir/lib/Analysis/Dominance.cpp:30
+template <bool IsPostDom>
+Block *
+DominanceInfoBase<IsPostDom>::findNearestCommonDominator(Block *a,
----------------
Keep the position of methods relative to the position in the class declaration, i.e. this should be below recalculate.


================
Comment at: mlir/lib/Analysis/Dominance.cpp:54
+    // Try to find the nearest common dominator of the two parent blocks.
+    return findNearestCommonDominator(parentA->getBlock(), parentB->getBlock());
+  }
----------------
It isn't clear to me that this is correct. What happens if the region containing `a` is a common ancestor of `b`? Seems like this should be doing something similar to `properlyDominates`, i.e. traversing the ancestors of `b` looking for the region containing `a`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75507