[mlir] improve the error message in expensive transform checks

Include the transform op being applied when reporting it using an invalidated
handle. This was missing previously and made it harder for the user to
understand where the handle is being used, especially if the transform script
included some sort of iteration.

Reviewed By: guraypp

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

diff --git a/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h b/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
index 61143cb..3da82f0 100644 (file)
--- a/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
+++ b/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
@@ -516,8 +516,9 @@ private:
 
   /// The mapping from invalidated handles to the error-reporting functions that
   /// describe when the handles were invalidated. Calling such a function emits
-  /// a user-visible diagnostic.
-  DenseMap<Value, std::function<void()>> invalidatedHandles;
+  /// a user-visible diagnostic with an additional note pointing to the given
+  /// location.
+  DenseMap<Value, std::function<void(Location)>> invalidatedHandles;
 
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
   /// A stack of nested regions that are being processed in the transform IR.

diff --git a/mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp b/mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
index 9ddaa08..f1dd41a 100644 (file)
--- a/mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
+++ b/mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
@@ -147,15 +147,18 @@ void transform::TransformState::recordHandleInvalidation(OpOperand &handle) {
         Operation *owner = handle.getOwner();
         unsigned operandNo = handle.getOperandNumber();
         invalidatedHandles[otherHandle] = [ancestorLoc, opLoc, owner, operandNo,
-                                           otherHandle]() {
-          InFlightDiagnostic diag =
-              owner->emitOpError()
-              << "invalidated the handle to payload operations nested in the "
-                 "payload operation associated with its operand #"
-              << operandNo;
-          diag.attachNote(ancestorLoc) << "ancestor op";
-          diag.attachNote(opLoc) << "nested op";
-          diag.attachNote(otherHandle.getLoc()) << "other handle";
+                                           otherHandle](Location currentLoc) {
+          InFlightDiagnostic diag = emitError(currentLoc)
+                                    << "op uses a handle invalidated by a "
+                                       "previously executed transform op";
+          diag.attachNote(otherHandle.getLoc()) << "handle to invalidated ops";
+          diag.attachNote(owner->getLoc())
+              << "invalidated by this transform op that consumes its operand #"
+              << operandNo
+              << " and invalidates handles to payload ops nested in payload "
+                 "ops associated with the consumed handle";
+          diag.attachNote(ancestorLoc) << "ancestor payload op";
+          diag.attachNote(opLoc) << "nested payload op";
         };
       }
     }
@@ -174,7 +177,7 @@ LogicalResult transform::TransformState::checkAndRecordHandleInvalidation(
     // If the operand uses an invalidated handle, report it.
     auto it = invalidatedHandles.find(target.get());
     if (it != invalidatedHandles.end())
-      return it->getSecond()(), failure();
+      return it->getSecond()(transform->getLoc()), failure();
 
     // Invalidate handles pointing to the operations nested in the operation
     // associated with the handle consumed by this operation.

diff --git a/mlir/test/Dialect/Transform/expensive-checks.mlir b/mlir/test/Dialect/Transform/expensive-checks.mlir
index 6340d1d..ab15197 100644 (file)
--- a/mlir/test/Dialect/Transform/expensive-checks.mlir
+++ b/mlir/test/Dialect/Transform/expensive-checks.mlir
@@ -1,8 +1,8 @@
 // RUN: mlir-opt --test-transform-dialect-interpreter='enable-expensive-checks=1' --split-input-file --verify-diagnostics %s
 
-// expected-note @below {{ancestor op}}
+// expected-note @below {{ancestor payload op}}
 func.func @func() {
-  // expected-note @below {{nested op}}
+  // expected-note @below {{nested payload op}}
   return
 }
 
@@ -17,11 +17,12 @@ transform.with_pdl_patterns {
 
   sequence %arg0 failures(propagate) {
   ^bb1(%arg1: !pdl.operation):
-    // expected-note @below {{other handle}}
+    // expected-note @below {{handle to invalidated ops}}
     %0 = pdl_match @return in %arg1
     %1 = get_closest_isolated_parent %0
-    // expected-error @below {{invalidated the handle to payload operations nested in the payload operation associated with its operand #0}}
+    // expected-note @below {{invalidated by this transform op that consumes its operand #0}}
     test_consume_operand %1
+    // expected-error @below {{op uses a handle invalidated by a previously executed transform op}}
     test_print_remark_at_operand %0, "remark"
   }
 }