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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added inline comments.


================
Comment at: mlir/lib/Analysis/Dominance.cpp:97
+
+  // If there is no match, we will not able to find a common dominator since
+  // both regions do not share a common parent region.
----------------
`not able` -> `not be able`


================
Comment at: mlir/lib/Analysis/Dominance.cpp:159
+
+    // If we could not find a valid block b then it is either a not a dominator
+    // or a post dominator.
----------------
`a not a` => `not a`


================
Comment at: mlir/lib/Analysis/Dominance.cpp:162
+    if (!b)
+      return IsPostDom;
 
----------------
You did not introduce this. Still, I do not get the post-dominates here. If `a` and `b` are both in subregions, like
```
top {
  op1 {
    a^:
  }
  op2 {
    b^:
  }
}
```
then the above code will not find a region in the ancestry of `b` that is also the region of `a`. Still, `a` does not post-dominate `b`. 


================
Comment at: mlir/test/lib/Transforms/TestDominance.cpp:49
+          return;
+        llvm::errs() << "Dom " << blockIds[block] << " -> "
+                     << blockIds[nestedBlock] << " = ";
----------------
The output of this is misleading to read. The `->` reads like the lhs dominates the rhs. Something like `Nearest(0, 1) = 2` would be easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75507