Improve the verifier diagnostic on dominance error

[mehdi_amini]

Summary

Address PR47937

[rriddle]

mlir/lib/IR/Verifier.cpp

312

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();
 }
 
+/// 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) {
     // 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";
+          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
     // 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}}