[Mlir-commits] [mlir] c21ee1a - Improve the verifier diagnostic on dominance error

[Mehdi Amini]

Author: Mehdi Amini
Date: 2020-12-16T22:05:17Z
New Revision: c21ee1a9426710eba5923062efd0e0b0789edd47

URL: https://github.com/llvm/llvm-project/commit/c21ee1a9426710eba5923062efd0e0b0789edd47
DIFF: https://github.com/llvm/llvm-project/commit/c21ee1a9426710eba5923062efd0e0b0789edd47.diff

LOG: Improve the verifier diagnostic on dominance error

Address PR47937

Differential Revision: https://reviews.llvm.org/D93361

Added: 
    

Modified: 
    mlir/lib/IR/Verifier.cpp
    mlir/test/IR/invalid.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp
index e214a654dbee..036919bd84b9 100644
--- a/mlir/lib/IR/Verifier.cpp
+++ b/mlir/lib/IR/Verifier.cpp
@@ -245,6 +245,56 @@ LogicalResult OperationVerifier::verifyOperation(Operation &op) {
   return success();
 }
 
+/// Attach a note to an in-flight diagnostic that provide more information about
+/// where an op operand is defined.
+static void attachNoteForOperandDefinition(InFlightDiagnostic &diag,
+                                           Operation &op, Value operand) {
+  if (auto *useOp = operand.getDefiningOp()) {
+    Diagnostic &note = diag.attachNote(useOp->getLoc());
+    note << "operand defined here";
+    Block *block1 = op.getBlock();
+    Block *block2 = useOp->getBlock();
+    Region *region1 = block1->getParent();
+    Region *region2 = block2->getParent();
+    if (block1 == block2)
+      note << " (op in the same block)";
+    else if (region1 == region2)
+      note << " (op in the same region)";
+    else if (region2->isProperAncestor(region1))
+      note << " (op in a parent region)";
+    else if (region1->isProperAncestor(region2))
+      note << " (op in a child region)";
+    else
+      note << " (op is neither in a parent nor in a child region)";
+    return;
+  }
+  // Block argument case.
+  Block *block1 = op.getBlock();
+  Block *block2 = operand.cast<BlockArgument>().getOwner();
+  Region *region1 = block1->getParent();
+  Region *region2 = block2->getParent();
+  Location loc = UnknownLoc::get(op.getContext());
+  if (block2->getParentOp())
+    loc = block2->getParentOp()->getLoc();
+  Diagnostic &note = diag.attachNote(loc);
+  if (!region2) {
+    note << " (block without parent)";
+    return;
+  }
+  if (block1 == block2)
+    llvm::report_fatal_error("Internal error in dominance verification");
+  int index = std::distance(region2->begin(), block2->getIterator());
+  note << "operand defined as a block argument (block #" << index;
+  if (region1 == region2)
+    note << " in the same region)";
+  else if (region2->isProperAncestor(region1))
+    note << " in a parent region)";
+  else if (region1->isProperAncestor(region2))
+    note << " in a child region)";
+  else
+    note << " neither in a parent nor in a child region)";
+}
+
 LogicalResult OperationVerifier::verifyDominance(Region &region) {
   // Verify the dominance of each of the held operations.
   for (Block &block : region) {
@@ -254,14 +304,14 @@ LogicalResult OperationVerifier::verifyDominance(Region &region) {
         // Check that operands properly dominate this use.
         for (unsigned operandNo = 0, e = op.getNumOperands(); operandNo != e;
              ++operandNo) {
-          auto operand = op.getOperand(operandNo);
+          Value operand = op.getOperand(operandNo);
           if (domInfo->properlyDominates(operand, &op))
             continue;
 
-          auto diag = op.emitError("operand #")
-                      << operandNo << " does not dominate this use";
-          if (auto *useOp = operand.getDefiningOp())
-            diag.attachNote(useOp->getLoc()) << "operand defined here";
+          InFlightDiagnostic diag = op.emitError("operand #")
+                                    << operandNo
+                                    << " does not dominate this use";
+          attachNoteForOperandDefinition(diag, op, operand);
           return failure();
         }
     // Recursively verify dominance within each operation in the

diff  --git a/mlir/test/IR/invalid.mlir b/mlir/test/IR/invalid.mlir
index 86245ad25c3b..4e6c950d637a 100644
--- a/mlir/test/IR/invalid.mlir
+++ b/mlir/test/IR/invalid.mlir
@@ -464,7 +464,49 @@ func @dominance_failure() {
   "foo"(%x) : (i32) -> ()    // expected-error {{operand #0 does not dominate this use}}
   br ^bb1
 ^bb1:
-  %x = "bar"() : () -> i32    // expected-note {{operand defined here}}
+  %x = "bar"() : () -> i32    // expected-note {{operand defined here (op in the same region)}}
+  return
+}
+
+// -----
+
+func @dominance_failure() {
+^bb0:
+  "foo"(%x) : (i32) -> ()    // expected-error {{operand #0 does not dominate this use}}
+  %x = "bar"() : () -> i32    // expected-note {{operand defined here (op in the same block)}}
+  br ^bb1
+^bb1:
+  return
+}
+
+// -----
+
+func @dominance_failure() {
+  "foo"() ({
+    "foo"(%x) : (i32) -> ()    // expected-error {{operand #0 does not dominate this use}}
+  }) : () -> ()
+  %x = "bar"() : () -> i32    // expected-note {{operand defined here (op in a parent region)}}
+  return
+}
+
+// -----
+
+func @dominance_failure() {  //  expected-note {{operand defined as a block argument (block #1 in the same region)}}
+^bb0:
+  br ^bb1(%x : i32)    // expected-error {{operand #0 does not dominate this use}}
+^bb1(%x : i32):
+  return
+}
+
+// -----
+
+func @dominance_failure() {  //  expected-note {{operand defined as a block argument (block #1 in a parent region)}}
+^bb0:
+  %f = "foo"() ({
+    "foo"(%x) : (i32) -> ()    // expected-error {{operand #0 does not dominate this use}}
+  }) : () -> (i32)
+  br ^bb1(%f : i32)
+^bb1(%x : i32):
   return
 }