Improve the verifier diagnostic on dominance error

[mehdi_amini]

Summary

Address PR47937

[rriddle]

mlir/lib/IR/Verifier.cpp

264

Can you create a helper function for this?

[mehdi_amini]

Extract into a helper

[rriddle]

mlir/lib/IR/Verifier.cpp

250

This function should be marked static. Only classes should go in anonymous namespaces.

[mehdi_amini]

Static

[rriddle]

Nice!

mlir/lib/IR/Verifier.cpp

248

nit: Use /// here for the comment.

288

nit: Add a space before the (.

[lattner]

This is awesome, thank you!

mlir/lib/IR/Verifier.cpp

if (!it->second) {
     return op.emitError("unregistered operation '")
            << op.getName() << "' found in dialect ('" << dialectPrefix
            << "') that does not allow unknown operations";
   }
 
   return success();
 }
 
 LogicalResult OperationVerifier::verifyDominance(Region &region) {
   // Verify the dominance of each of the held operations.
   for (Block &block : region) {
     // Dominance is only meaningful inside reachable blocks.
     if (domInfo->isReachableFromEntry(&block))
       for (Operation &op : block)
         // 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";
+          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)";
+          } else {
+            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)";
+            } else {
+              note << "operand defined as a block argument";
+              if (block1 == block2)
+                llvm::report_fatal_error(
+                    "Internal error in dominance verification");
+              int index =
+                  std::distance(region2->begin(), block2->getIterator());
+              note << "(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)";
+            }
+          }
           return failure();
         }
     // Recursively verify dominance within each operation in the
     // block, even if the block itself is not reachable, or we are in
     // a region which doesn't respect dominance.
     for (Operation &op : block)
       if (failed(verifyDominanceOfContainedRegions(op)))
         return failure();

mlir/test/IR/invalid.mlir

 
 // -----
 
 func @dominance_failure() {
 ^bb0:
   "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
 }
 
 // -----
 
 func @return_type_mismatch() -> i32 {
   %0 = "foo"() : ()->f32
   return %0 : f32  // expected-error {{type of return operand 0 ('f32') doesn't match function result type ('i32') in function @return_type_mismatch}}