[llvm-branch-commits] [mlir] [mlir] reduce excessive verification in transform (PR #192653)

[via llvm-branch-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: Oleksandr "Alex" Zinenko (ftynse)

<details>
<summary>Changes</summary>

`mergeSymbolsInto` called by the transform interpreter for named sequence management was calling a full verifier after renaming symbols. The renaming could have potentially broken symbol table-related invariants, but not really anything else. Only verify the symbol table-related invariants intead.

---
Full diff: https://github.com/llvm/llvm-project/pull/192653.diff


2 Files Affected:

- (modified) mlir/lib/Dialect/Transform/IR/Utils.cpp (+8-4) 
- (modified) mlir/test/Dialect/Transform/normal-forms.mlir (+4-5) 


``````````diff
diff --git a/mlir/lib/Dialect/Transform/IR/Utils.cpp b/mlir/lib/Dialect/Transform/IR/Utils.cpp
index 773eb138cdf62..4c2f0ab376231 100644
--- a/mlir/lib/Dialect/Transform/IR/Utils.cpp
+++ b/mlir/lib/Dialect/Transform/IR/Utils.cpp
@@ -168,11 +168,12 @@ transform::detail::mergeSymbolsInto(Operation *target,
     }
   }
 
-  // TODO: This duplicates pass infrastructure. We should split this pass into
-  //       several and let the pass infrastructure do the verification.
+  // We only modified symbols above, so there is no need to verify everything
+  // again, just the symbol table.
   for (auto *op : SmallVector<Operation *>{target, *other}) {
-    if (failed(mlir::verify(op)))
-      return op->emitError() << "failed to verify input op after renaming";
+    if (failed(mlir::detail::verifySymbolTable(op)))
+      return op->emitError()
+             << "failed to verify symbol table after symbol renaming";
   }
 
   // Step 2:
@@ -234,6 +235,9 @@ transform::detail::mergeSymbolsInto(Operation *target,
     }
   }
 
+  // Need full verification here because merging/inlining may have broken some
+  // nesting invariants that were not broken in the sources.
+  // TODO: implement and use InlinerDialectInterface to avoid this check.
   if (failed(mlir::verify(target)))
     return target->emitError()
            << "failed to verify target op after merging symbols";
diff --git a/mlir/test/Dialect/Transform/normal-forms.mlir b/mlir/test/Dialect/Transform/normal-forms.mlir
index dfc6e4bdeb7b8..e66670e54bc23 100644
--- a/mlir/test/Dialect/Transform/normal-forms.mlir
+++ b/mlir/test/Dialect/Transform/normal-forms.mlir
@@ -139,16 +139,15 @@ transform.payload attributes {
 
 // We have surprisingly many invocations of the verifier here:
 //  1. after the initial parsing (reasonable)
-//  2. in transform::detail::mergeSymbolsInto (looks excessive)
-//  3. also in transform::detail::mergeSymbolsInto (has a TODO to be removed)
-//  4. after the transform interpreter pass (reasonable)
-//  5. before printing (generally reasonable, but would be nice to avoid if 
+//  2. also in transform::detail::mergeSymbolsInto (has a TODO to be removed)
+//  3. after the transform interpreter pass (reasonable)
+//  4. before printing (generally reasonable, but would be nice to avoid if
 //     the IR is known-verified after by the pass manager).
 // Notably this doesn't include an extra run from checkPayload, which is
 // what we intend to test here.
 
 // CHECK: transform.payload
-// CHECK-SAME: test.counting_normal_form_count = 5 : i64
+// CHECK-SAME: test.counting_normal_form_count = 4 : i64
 
 module attributes {transform.with_named_sequence} {
   transform.payload attributes {

``````````

</details>


https://github.com/llvm/llvm-project/pull/192653