[Mlir-commits] [mlir] [mlir] targeted verification for transform "inlining" (PR #192956)

[Martin Paul Lücke]

================
@@ -227,21 +263,57 @@ transform::detail::mergeSymbolsInto(Operation *target,
       assert(targetSymbolTable.lookup(funcOp.getName()) == collidingFuncOp);
 
       // Do the actual merging.
-      {
-        InFlightDiagnostic diag = mergeInto(funcOp, collidingFuncOp);
-        if (failed(diag))
-          return diag;
-      }
+      if (failed(mergeInto(funcOp, collidingFuncOp)))
+        return failure();
     }
   }
 
-  // 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";
+  // Symbol merging only moves callable ops between symbol tables; it does not
+  // alter the bodies that were already valid in the source modules. The only
+  // invariants that may newly be violated after merging are:
+  //   1. a call now refers to a callee whose body is structurally not legal to
+  //      inline at the call site (caught by the transform dialect's
+  //      `DialectInlinerInterface` implementation), or
+  //   2. the merged call graph contains a recursive cycle, which is forbidden
+  //      for `transform.named_sequence` callables (caught by the shared
+  //      `verifyNoRecursionInCallGraph` helper).
+  // Use the inliner interface methods directly (without running the inlining
+  // pass) to validate (1), and reuse the dialect's call-graph verifier for
+  // (2). The call graph builder requires call/callable ops to be well-formed,
+  // so pre-verify them here without recursing into their bodies.
+  WalkResult preVerify = target->walk([](Operation *nested) {
+    if (!isa<CallableOpInterface, CallOpInterface>(nested))
+      return WalkResult::advance();
+    if (failed(mlir::verify(nested, /*verifyRecursively=*/false)))
+      return WalkResult::interrupt();
+    return WalkResult::advance();
+  });
+  if (preVerify.wasInterrupted())
+    return failure();
+
+  InlinerInterface inliner(target->getContext());
+  WalkResult inlineCheck = target->walk([&](CallOpInterface call) {
+    Operation *callable = nullptr;
+    CallInterfaceCallable callee = call.getCallableForCallee();
+    if (auto symRef = dyn_cast<SymbolRefAttr>(callee))
+      callable = targetSymbolTable.lookup(symRef.getLeafReference());
----------------
martin-luecke wrote:

`getLeafReference()` + top-level lookup won't correctly resolve nested symbol refs like `@foo_module::@__transform_main` (see [interpreter-entry-point-2.mlir](https://github.com/llvm/llvm-project/blob/d3ee88b18e225ab00a0dc5a8e85b472c4d15b922/mlir/test/Dialect/Transform/interpreter-entry-point-2.mlir#L4)): it would look up `__transform_main` in the top-level symbol table, finding either a different op with that name or null. Not a problem today because a wrong match still passes the `isLegalToInline` check (which only tests isa<NamedSequenceOp>, true for all transform callables), and a null match is silently skipped. If the legality check ever becomes sensitive to which callable is resolved, this would need `lookupNearestSymbolFrom` or `SymbolTable::lookupSymbolIn`.

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