[Mlir-commits] [mlir] 59a640a - [mlir] improve the error message in expensive transform checks

[Alex Zinenko]

Author: Alex Zinenko
Date: 2022-09-13T16:58:52+02:00
New Revision: 59a640a3813f28705751f000e1172067955c0585

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

LOG: [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

Added: 
    

Modified: 
    mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
    mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
    mlir/test/Dialect/Transform/expensive-checks.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h b/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
index 61143cbf8a6fc..3da82f01ba2a3 100644
--- a/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
+++ b/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
@@ -516,8 +516,9 @@ class TransformState {
 
   /// 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 9ddaa088e0af1..f1dd41a4a7619 100644
--- 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 6340d1d250705..ab151979aab9e 100644
--- 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"
   }
 }