Skip to content

Conversation

matthias-springer
Copy link
Member

@matthias-springer matthias-springer commented Nov 8, 2024

Remove properlyDominatesImpl and implement the functionality in properlyDominates directly.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2024

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

Remove properlyDominatesImpl and implement the functionality in properlyDominates directly.

Depends on #115413.


Full diff: https://github.com/llvm/llvm-project/pull/115430.diff

2 Files Affected:

  • (modified) mlir/include/mlir/IR/Dominance.h (+1-10)
  • (modified) mlir/lib/IR/Dominance.cpp (+4-4)
diff --git a/mlir/include/mlir/IR/Dominance.h b/mlir/include/mlir/IR/Dominance.h
index 95c99bd59f7b2f..15b033ec854fde 100644
--- a/mlir/include/mlir/IR/Dominance.h
+++ b/mlir/include/mlir/IR/Dominance.h
@@ -147,9 +147,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 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.
@@ -187,13 +185,6 @@ class DominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/false> {
   bool properlyDominates(Block *a, Block *b) const {
     return super::properlyDominates(a, b);
   }
-
-private:
-  // Return true if operation A properly dominates operation B.  The
-  /// 'enclosingOpOk' flag says whether we should return true if the b op is
-  /// enclosed by a region on 'A'.
-  bool properlyDominatesImpl(Operation *a, Operation *b,
-                             bool enclosingOpOk) const;
 };
 
 /// A class for computing basic postdominance information.
diff --git a/mlir/lib/IR/Dominance.cpp b/mlir/lib/IR/Dominance.cpp
index 31f7e7dbc925ce..bdd0febfa546e0 100644
--- a/mlir/lib/IR/Dominance.cpp
+++ b/mlir/lib/IR/Dominance.cpp
@@ -230,7 +230,7 @@ bool DominanceInfoBase<IsPostDom>::properlyDominates(Block *a, Block *b) const {
   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 == nullptr)
+    if (!b)
       return false;
 
     // Check to see if the ancestor of `b` is the same block as `a`.  A properly
@@ -266,8 +266,8 @@ template class detail::DominanceInfoBase</*IsPostDom=*/false>;
 /// Return true if operation `a` properly dominates operation `b`.  The
 /// 'enclosingOpOk' flag says whether we should return true if the `b` op is
 /// enclosed by a region on 'a'.
-bool DominanceInfo::properlyDominatesImpl(Operation *a, Operation *b,
-                                          bool enclosingOpOk) const {
+bool DominanceInfo::properlyDominates(Operation *a, Operation *b,
+                                      bool enclosingOpOk) const {
   Block *aBlock = a->getBlock(), *bBlock = b->getBlock();
   assert(aBlock && bBlock && "operations must be in a block");
 
@@ -319,7 +319,7 @@ bool DominanceInfo::properlyDominates(Value a, Operation *b) const {
 
   // `a` properlyDominates `b` if the operation defining `a` properlyDominates
   // `b`, but `a` does not itself enclose `b` in one of its regions.
-  return properlyDominatesImpl(a.getDefiningOp(), b, /*enclosingOpOk=*/false);
+  return properlyDominates(a.getDefiningOp(), b, /*enclosingOpOk=*/false);
 }
 
 //===----------------------------------------------------------------------===//

@matthias-springer matthias-springer force-pushed the users/matthias-springer/properly_dominate branch 2 times, most recently from 68c43c6 to 2052c65 Compare November 10, 2024 06:11
Base automatically changed from users/matthias-springer/properly_dominate to main November 10, 2024 06:20
@matthias-springer matthias-springer force-pushed the users/matthias-springer/minor_dom_cleanup branch from 58fbba4 to f997040 Compare November 10, 2024 06:21
@matthias-springer matthias-springer force-pushed the users/matthias-springer/minor_dom_cleanup branch from f997040 to 8bc07a5 Compare November 11, 2024 02:44
@matthias-springer matthias-springer merged commit 01dcc41 into main Nov 12, 2024
8 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/minor_dom_cleanup branch November 12, 2024 09:57
matthias-springer added a commit that referenced this pull request Nov 12, 2024
…ation (#115433)

The implementations of `DominanceInfo::properlyDominates` and
`PostDominanceInfo::properlyPostDominates` are almost identical: only
one line of code is different (apart from the missing `enclosingOpOk`
flag). Define the function in `DominanceInfoBase` to avoid the code
duplication.

Also rename the helper in `DominanceInfoBase` to
`properlyDominatesImpl`.

Note: This commit is not marked as NFC because
`PostDominanceInfo::properlyPostDominates` now also has an
`enclosingOpOk` argument.

Depends on #115430.
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
Remove `properlyDominatesImpl` and implement the functionality in
`properlyDominates` directly.
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
…ation (llvm#115433)

The implementations of `DominanceInfo::properlyDominates` and
`PostDominanceInfo::properlyPostDominates` are almost identical: only
one line of code is different (apart from the missing `enclosingOpOk`
flag). Define the function in `DominanceInfoBase` to avoid the code
duplication.

Also rename the helper in `DominanceInfoBase` to
`properlyDominatesImpl`.

Note: This commit is not marked as NFC because
`PostDominanceInfo::properlyPostDominates` now also has an
`enclosingOpOk` argument.

Depends on llvm#115430.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants