[mlir][IR][NFC] DominanceInfo: Share same impl for block/op dominance
#115587
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Matthias Springer (matthias-springer) ChangesThe Note: A subsequent commit will add a new public Depends on #115433. Full diff: https://github.com/llvm/llvm-project/pull/115587.diff 2 Files Affected:
diff --git a/mlir/include/mlir/IR/Dominance.h b/mlir/include/mlir/IR/Dominance.h
index 933ec09c5ede29..a6b2475e12b1c6 100644
--- a/mlir/include/mlir/IR/Dominance.h
+++ b/mlir/include/mlir/IR/Dominance.h
@@ -113,12 +113,12 @@ class DominanceInfoBase {
llvm::PointerIntPair<DomTree *, 1, bool>
getDominanceInfo(Region *region, bool needsDomTree) const;
- /// Return "true" if the specified block A properly (post)dominates block B.
- bool properlyDominatesImpl(Block *a, Block *b) const;
-
- /// Return "true" if the specified op A properly (post)dominates op B.
- bool properlyDominatesImpl(Operation *a, Operation *b,
- bool enclosingOpOk = true) const;
+ /// Return "true" if block iterator A properly (post)dominates block iterator
+ /// B. If `enclosingOk` is set, A is considered to (post)dominate B if A
+ /// encloses B.
+ bool properlyDominatesImpl(Block *aBlock, Block::iterator aIt, Block *bBlock,
+ Block::iterator bIt,
+ bool enclosingOk = true) const;
/// A mapping of regions to their base dominator tree and a cached
/// "hasSSADominance" bit. This map does not contain dominator trees for
@@ -151,9 +151,7 @@ class DominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/false> {
/// The `enclosingOpOk` flag says whether we should return true if the B op
/// is enclosed by a region on A.
bool properlyDominates(Operation *a, Operation *b,
- bool enclosingOpOk = true) const {
- return super::properlyDominatesImpl(a, b, enclosingOpOk);
- }
+ bool enclosingOpOk = true) const;
/// Return true if operation A dominates operation B, i.e. if A and B are the
/// same operation or A properly dominates B.
@@ -188,9 +186,7 @@ class DominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/false> {
/// Graph regions have only a single block. To be consistent with "proper
/// dominance" of ops, the single block is considered to properly dominate
/// itself in a graph region.
- bool properlyDominates(Block *a, Block *b) const {
- return super::properlyDominatesImpl(a, b);
- }
+ bool properlyDominates(Block *a, Block *b) const;
};
/// A class for computing basic postdominance information.
@@ -200,9 +196,7 @@ class PostDominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/true> {
/// Return true if operation A properly postdominates operation B.
bool properlyPostDominates(Operation *a, Operation *b,
- bool enclosingOpOk = true) {
- return super::properlyDominatesImpl(a, b, enclosingOpOk);
- }
+ bool enclosingOpOk = true);
/// Return true if operation A postdominates operation B.
bool postDominates(Operation *a, Operation *b) {
@@ -210,9 +204,7 @@ class PostDominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/true> {
}
/// Return true if the specified block A properly postdominates block B.
- bool properlyPostDominates(Block *a, Block *b) {
- return super::properlyDominatesImpl(a, b);
- }
+ bool properlyPostDominates(Block *a, Block *b);
/// Return true if the specified block A postdominates block B.
bool postDominates(Block *a, Block *b) {
diff --git a/mlir/lib/IR/Dominance.cpp b/mlir/lib/IR/Dominance.cpp
index 406e0f2d62d640..337a1b7af9d40f 100644
--- a/mlir/lib/IR/Dominance.cpp
+++ b/mlir/lib/IR/Dominance.cpp
@@ -213,61 +213,73 @@ DominanceInfoBase<IsPostDom>::findNearestCommonDominator(Block *a,
return getDomTree(a->getParent()).findNearestCommonDominator(a, b);
}
-/// Return true if the specified block A properly dominates block B.
-template <bool IsPostDom>
-bool DominanceInfoBase<IsPostDom>::properlyDominatesImpl(Block *a,
- Block *b) const {
- assert(a && b && "null blocks not allowed");
+/// Returns the given block iterator if it lies within the region region.
+/// Otherwise, otherwise finds the ancestor of the given block iterator that
+/// lies within the given region. Returns and "empty" iterator if the latter
+/// fails.
+///
+/// Note: This is a variant of Region::findAncestorOpInRegion that operates on
+/// block iterators instead of ops.
+static std::pair<Block *, Block::iterator>
+findAncestorIteratorInRegion(Region *r, Block *b, Block::iterator it) {
+ // Case 1: The iterator lies within the region region.
+ if (b->getParent() == r)
+ return std::make_pair(b, it);
+
+ // Otherwise: Find ancestor iterator. Bail if we run out of parent ops.
+ Operation *parentOp = b->getParentOp();
+ if (!parentOp)
+ return std::make_pair(static_cast<Block *>(nullptr), Block::iterator());
+ Operation *op = r->findAncestorOpInRegion(*parentOp);
+ if (!op)
+ return std::make_pair(static_cast<Block *>(nullptr), Block::iterator());
+ return std::make_pair(op->getBlock(), op->getIterator());
+}
- // A block dominates, but does not properly dominate, itself unless this
- // is a graph region.
+/// Given two iterators into the same block, return "true" if `a` is before `b.
+/// Note: This is a variant of Operation::isBeforeInBlock that operates on
+/// block iterators instead of ops.
+static bool isBeforeInBlock(Block *block, Block::iterator a,
+ Block::iterator b) {
if (a == b)
- return !hasSSADominance(a);
-
- // If both blocks are not in the same region, `a` properly dominates `b` if
- // `b` is defined in an operation region that (recursively) ends up being
- // dominated by `a`. Walk up the list of containers enclosing B.
- Region *regionA = a->getParent();
- if (regionA != b->getParent()) {
- b = regionA ? regionA->findAncestorBlockInRegion(*b) : nullptr;
- // If we could not find a valid block b then it is a not a dominator.
- if (!b)
- return false;
-
- // Check to see if the ancestor of `b` is the same block as `a`. A properly
- // dominates B if it contains an op that contains the B block.
- if (a == b)
- return true;
- }
-
- // Otherwise, they are two different blocks in the same region, use DomTree.
- return getDomTree(regionA).properlyDominates(a, b);
+ return false;
+ if (a == block->end())
+ return false;
+ if (b == block->end())
+ return true;
+ return a->isBeforeInBlock(&*b);
}
template <bool IsPostDom>
bool DominanceInfoBase<IsPostDom>::properlyDominatesImpl(
- Operation *a, Operation *b, bool enclosingOpOk) const {
- Block *aBlock = a->getBlock(), *bBlock = b->getBlock();
- assert(aBlock && bBlock && "operations must be in a block");
+ Block *aBlock, Block::iterator aIt, Block *bBlock, Block::iterator bIt,
+ bool enclosingOk) const {
+ assert(aBlock && bBlock && "expected non-null blocks");
- // An operation (pos)dominates, but does not properly (pos)dominate, itself
- // unless this is a graph region.
- if (a == b)
+ // A block iterator (post)dominates, but does not properly (post)dominate,
+ // itself unless this is a graph region.
+ if (aBlock == bBlock && aIt == bIt)
return !hasSSADominance(aBlock);
- // If these ops are in different regions, then normalize one into the other.
+ // If the iterators are in different regions, then normalize one into the
+ // other.
Region *aRegion = aBlock->getParent();
if (aRegion != bBlock->getParent()) {
- // Scoot up b's region tree until we find an operation in A's region that
+ // Scoot up b's region tree until we find a location in A's region that
// encloses it. If this fails, then we know there is no (post)dom relation.
- b = aRegion ? aRegion->findAncestorOpInRegion(*b) : nullptr;
- if (!b)
+ if (!aRegion) {
+ bBlock = nullptr;
+ bIt = Block::iterator();
+ } else {
+ std::tie(bBlock, bIt) =
+ findAncestorIteratorInRegion(aRegion, bBlock, bIt);
+ assert(bBlock->getParent() == aRegion && "expected block in regionA");
+ }
+ if (!bBlock)
return false;
- bBlock = b->getBlock();
- assert(bBlock->getParent() == aRegion);
// If 'a' encloses 'b', then we consider it to (post)dominate.
- if (a == b && enclosingOpOk)
+ if (aBlock == bBlock && aIt == bIt && enclosingOk)
return true;
}
@@ -279,9 +291,9 @@ bool DominanceInfoBase<IsPostDom>::properlyDominatesImpl(
if (!hasSSADominance(aBlock))
return true;
if constexpr (IsPostDom) {
- return b->isBeforeInBlock(a);
+ return isBeforeInBlock(aBlock, bIt, aIt);
} else {
- return a->isBeforeInBlock(b);
+ return isBeforeInBlock(aBlock, aIt, bIt);
}
}
@@ -309,6 +321,18 @@ template class detail::DominanceInfoBase</*IsPostDom=*/false>;
// DominanceInfo
//===----------------------------------------------------------------------===//
+bool DominanceInfo::properlyDominates(Operation *a, Operation *b,
+ bool enclosingOpOk) const {
+ return super::properlyDominatesImpl(a->getBlock(), a->getIterator(),
+ b->getBlock(), b->getIterator(),
+ enclosingOpOk);
+}
+
+bool DominanceInfo::properlyDominates(Block *a, Block *b) const {
+ return super::properlyDominatesImpl(a, a->begin(), b, b->begin(),
+ /*enclosingOk=*/true);
+}
+
/// Return true if the `a` value properly dominates operation `b`, i.e if the
/// operation that defines `a` properlyDominates `b` and the operation that
/// defines `a` does not contain `b`.
@@ -322,3 +346,19 @@ bool DominanceInfo::properlyDominates(Value a, Operation *b) const {
// `b`, but `a` does not itself enclose `b` in one of its regions.
return properlyDominates(a.getDefiningOp(), b, /*enclosingOpOk=*/false);
}
+
+//===----------------------------------------------------------------------===//
+// PostDominanceInfo
+//===----------------------------------------------------------------------===//
+
+bool PostDominanceInfo::properlyPostDominates(Operation *a, Operation *b,
+ bool enclosingOpOk) {
+ return super::properlyDominatesImpl(a->getBlock(), a->getIterator(),
+ b->getBlock(), b->getIterator(),
+ enclosingOpOk);
+}
+
+bool PostDominanceInfo::properlyPostDominates(Block *a, Block *b) {
+ return super::properlyDominatesImpl(a, a->end(), b, b->end(),
+ /*enclosingOk=*/true);
+}
|
matthias-springer
force-pushed
the
users/matthias-springer/block_dom
branch
from
3911ab1 to
9979c63
Compare
matthias-springer
added a commit
that referenced
this pull request
Nov 9, 2024
Same as `DominanceInfo`, all functions should be `const`. Depends on #115587.
matthias-springer
force-pushed
the
users/matthias-springer/dominance_deduplicate
branch
from
26e5b4f to
d6ab91c
Compare
matthias-springer
force-pushed
the
users/matthias-springer/block_dom
branch
from
9979c63 to
247e4db
Compare
matthias-springer
added a commit
that referenced
this pull request
Nov 10, 2024
Same as `DominanceInfo`, all functions should be `const`. Depends on #115587.
matthias-springer
force-pushed
the
users/matthias-springer/dominance_deduplicate
branch
from
d6ab91c to
213a01b
Compare
matthias-springer
force-pushed
the
users/matthias-springer/block_dom
branch
from
247e4db to
57bd391
Compare
matthias-springer
force-pushed
the
users/matthias-springer/dominance_deduplicate
branch
from
213a01b to
4c303e1
Compare
Base automatically changed from
users/matthias-springer/dominance_deduplicate
to
main
November 12, 2024 10:58
matthias-springer
force-pushed
the
users/matthias-springer/block_dom
branch
from
57bd391 to
c56dc33
Compare
Member
Author
|
This PR is no longer needed for the dialect conversion changes, but it could still be a nice cleanup, as it combines two functions that do a very similar ancestor lookup into a single function that is reused in both cases. If you agree, please leave a review. Otherwise, I'm going to close this PR after some time... |
matthias-springer
changed the base branch from
main
to
users/matthias-springer/1n_conversion_value_mapping
matthias-springer
force-pushed
the
users/matthias-springer/block_dom
branch
from
c56dc33 to
b484c72
Compare
Member
Author
I have another use case for this: this is needed after all in the dialect conversion driver when computing an insertion point of an N:1 source materialization. These materializations are stored in the |
matthias-springer
force-pushed
the
users/matthias-springer/1n_conversion_value_mapping
branch
from
53f97f5 to
41a9dde
Compare
matthias-springer
requested review from
PeimingLiu,
aartbik,
banach-space,
dcaballe,
hanhanW and
yinying-lisa-li
as code owners
matthias-springer
requested review from
nicolasvasilache and
rengolin
as code owners
matthias-springer
force-pushed
the
users/matthias-springer/1n_conversion_value_mapping
branch
2 times, most recently
from
680692f to
5b49dce
Compare
matthias-springer
force-pushed
the
users/matthias-springer/1n_conversion_value_mapping
branch
3 times, most recently
from
488d8be to
209d922
Compare
Base automatically changed from
users/matthias-springer/1n_conversion_value_mapping
to
main
January 3, 2025 15:11
The `properlyDominates` implementations for blocks and ops are very similar. This commit replaces them with a single implementation that operates on block iterators. That implementation can be used to implement both `properlyDominates` variants. Note: A subsequent commit will add a new public `properlyDominates` overload that accepts block iterators. That functionality can then be used to find a valid insertion point at which a range of values is defined (by utilizing post dominance). Depends on #115433.
matthias-springer
force-pushed
the
users/matthias-springer/block_dom
branch
from
b484c72 to
077584f
Compare
matthias-springer
requested review from
Mogball
and removed request for
PeimingLiu,
aartbik,
banach-space,
dcaballe,
hanhanW,
nicolasvasilache and
yinying-lisa-li
Mogball
approved these changes
Jan 3, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/199/builds/634 Here is the relevant piece of the build log for the reference |
Member
Author
|
The build bot failure is not related to this commit. It is a flaky test. I am looking into it. |
Member
Author
|
Note: The flaky test should be fixed by #121662. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
properlyDominatesimplementations for blocks and ops are very similar. This commit replaces them with a single implementation that operates on block iterators. That implementation can be used to implement bothproperlyDominatesvariants.Before:
After:
Note: A subsequent commit will add a new public
properlyDominatesoverload that accepts block iterators. That functionality can then be used to find a valid insertion point at which a range of values is defined (by utilizing post dominance).