[Mlir-commits] [mlir] [mlir][IR] Fix inconsistency in block/op dominance (PR #115413)

[Matthias Springer]

https://github.com/matthias-springer updated https://github.com/llvm/llvm-project/pull/115413

>From e8c5e298de5bfb8c724e8c2b52e8dd1fe0f87f30 Mon Sep 17 00:00:00 2001
From: Matthias Springer <mspringer at nvidia.com>
Date: Fri, 8 Nov 2024 03:31:29 +0100
Subject: [PATCH] [mlir][IR] Fix inconsistency in block/op dominance

An operation is considered to properly dominate itself in a graph region. That's because there is no concept of "dominance" in a graph region. (`dominates` returns "true" for all pairs of ops in the same block.)

Previously, a block was *not* considered to dominate itself in a graph region. This commit fixes thise asymmetry between ops and blocks: both are now properly dominating themselves in a graph region.
---
 mlir/include/mlir/IR/Dominance.h | 15 ++++++++++-----
 mlir/lib/IR/Dominance.cpp        | 10 ++++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/mlir/include/mlir/IR/Dominance.h b/mlir/include/mlir/IR/Dominance.h
index 2536ce585b3fdd..95c99bd59f7b2f 100644
--- a/mlir/include/mlir/IR/Dominance.h
+++ b/mlir/include/mlir/IR/Dominance.h
@@ -141,8 +141,8 @@ class DominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/false> {
   /// are in the same block and A properly dominates B within the block, or if
   /// the block that contains A properly dominates the block that contains B. In
   /// an SSACFG region, Operation A dominates Operation B in the same block if A
-  /// preceeds B. In a Graph region, all operations in a block dominate all
-  /// other operations in the same block.
+  /// preceeds B. In a Graph region, all operations in a block properly dominate
+  /// all operations in the same block.
   ///
   /// The `enclosingOpOk` flag says whether we should return true if the B op
   /// is enclosed by a region on A.
@@ -176,9 +176,14 @@ class DominanceInfo : public detail::DominanceInfoBase</*IsPostDom=*/false> {
   /// Return true if the specified block A properly dominates block B, i.e.: if
   /// block A contains block B, or if the region which contains block A also
   /// contains block B or some parent of block B and block A dominates that
-  /// block in that kind of region. In an SSACFG region, block A dominates
-  /// block B if all control flow paths from the entry block to block B flow
-  /// through block A. In a Graph region, all blocks dominate all other blocks.
+  /// block in that kind of region.
+  ///
+  /// In an SSACFG region, block A dominates block B if all control flow paths
+  /// from the entry block to block B flow through block A.
+  ///
+  /// 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::properlyDominates(a, b);
   }
diff --git a/mlir/lib/IR/Dominance.cpp b/mlir/lib/IR/Dominance.cpp
index 2b138ae223546e..31f7e7dbc925ce 100644
--- a/mlir/lib/IR/Dominance.cpp
+++ b/mlir/lib/IR/Dominance.cpp
@@ -34,7 +34,8 @@ DominanceInfoBase<IsPostDom>::~DominanceInfoBase() {
     delete entry.second.getPointer();
 }
 
-template <bool IsPostDom> void DominanceInfoBase<IsPostDom>::invalidate() {
+template <bool IsPostDom>
+void DominanceInfoBase<IsPostDom>::invalidate() {
   for (auto entry : dominanceInfos)
     delete entry.second.getPointer();
   dominanceInfos.clear();
@@ -217,9 +218,10 @@ template <bool IsPostDom>
 bool DominanceInfoBase<IsPostDom>::properlyDominates(Block *a, Block *b) const {
   assert(a && b && "null blocks not allowed");
 
-  // A block dominates itself but does not properly dominate itself.
+  // A block dominates, but does not properly dominate, itself unless this
+  // is a graph region.
   if (a == b)
-    return false;
+    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
@@ -269,7 +271,7 @@ bool DominanceInfo::properlyDominatesImpl(Operation *a, Operation *b,
   Block *aBlock = a->getBlock(), *bBlock = b->getBlock();
   assert(aBlock && bBlock && "operations must be in a block");
 
-  // An instruction dominates, but does not properlyDominate, itself unless this
+  // An operation dominates, but does not properly dominate, itself unless this
   // is a graph region.
   if (a == b)
     return !hasSSADominance(aBlock);