The Dominance analysis currently misses a utility function to find the nearest common dominator of two given blocks. This is required for a huge variety of different control-flow analyses and transformations. This commit adds this function and moves the getNode function from DominanceInfo to DominanceInfoBase, as it also works for post dominators.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Analysis/Dominance.cpp | ||
---|---|---|
47 | What about the case where one block is in an ancestor region of the other block. Something like bb0^: "some.op"() { bb1^: // some code } |
@herhut Regarding your example that one block is in a nested region of the second block: you are right that you will not get the nearest common dominator in this case. In order to add support for these cases, we will have to check whether one region is nested into the other one. May we use the 'isProperAncestor' function to check whether a is nested into b or vice versa?
Thanks for the patch! I added a few comments.
mlir/lib/Analysis/Dominance.cpp | ||
---|---|---|
31 | Keep the position of methods relative to the position in the class declaration, i.e. this should be below recalculate. | |
55 | 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. |
mlir/lib/Analysis/Dominance.cpp | ||
---|---|---|
97 | not able -> not be able | |
159 | a not a => not a | |
162 | 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. | |
mlir/test/lib/Transforms/TestDominance.cpp | ||
50 | 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. |
mlir/lib/Analysis/Dominance.cpp | ||
---|---|---|
162 | Nested dominance has a lot of trickyness. We generally model dominance here as being able to flow-in, but not flow-out. So in the above, a^ can't dominate anything defined above its region inside of op1. That being said, different users want different things so we could likely expand the API surface area to handle the different cases. |
mlir/lib/Analysis/Dominance.cpp | ||
---|---|---|
162 | @rriddle I was more wondering about the case of post-dominance here. In the current implementation, a post-dominates b if I read the code right. That seems wrong to me. a also does not dominate b.
I did not understand that comment. |
As this does not change the semantics of what is currently implemented, I am fine with this to land.
Keep the position of methods relative to the position in the class declaration, i.e. this should be below recalculate.