[Mlir-commits] [mlir] [mlir] Fix crash in dialect conversion for detached root ops (PR #185068)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Mehdi Amini (joker-eph)

<details>
<summary>Changes</summary>

When running dialect conversion with --no-implicit-module, the root op is parsed without a wrapping module and then detached from its temporary parsing block (block == nullptr). If a conversion pattern erases or replaces this detached root op, ReplaceOperationRewrite::commit() would crash trying to call op->getBlock()->getOperations().remove(op) on a null block pointer.

Fix this in two complementary ways:

1. Add a null check in ReplaceOperationRewrite::commit() so that unlinking a detached op is silently skipped (there is no parent block to unlink from).

2. In OperationConverter::applyConversion(), track which input ops are detached before legalization begins. After a successful legalization, check whether any of those detached ops were replaced or erased. If so, roll back the rewrites and emit a clear error diagnostic rather than crashing.

Also add a regression test in FuncToSPIRV that reproduces the original crash:
  mlir-opt --no-implicit-module --convert-func-to-spirv

Fixes #<!-- -->60491
Assisted-by: Claude Code

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


2 Files Affected:

- (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+30-1) 
- (added) mlir/test/Conversion/FuncToSPIRV/func-to-spirv-no-implicit-module.mlir (+9) 


``````````diff
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 1dfa2103f57a7..79312391b0a4e 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1306,7 +1306,11 @@ void ReplaceOperationRewrite::commit(RewriterBase &rewriter) {
 
   // Do not erase the operation yet. It may still be referenced in `mapping`.
   // Just unlink it for now and erase it during cleanup.
-  op->getBlock()->getOperations().remove(op);
+  // Note: The op may not have a parent block if it was detached (e.g., when
+  // dialect conversion is applied to a top-level op with no parent block.
+  // In that case, there is nothing to unlink.
+  if (Block *block = op->getBlock())
+    block->getOperations().remove(op);
 }
 
 void ReplaceOperationRewrite::rollback() {
@@ -3459,6 +3463,16 @@ LogicalResult ConversionPatternRewriter::legalize(Region *r) {
 LogicalResult OperationConverter::applyConversion(ArrayRef<Operation *> ops) {
   // Convert each operation and discard rewrites on failure.
   ConversionPatternRewriterImpl &rewriterImpl = rewriter.getImpl();
+
+  // Track which root ops are detached (not in any block). If a detached root
+  // op is replaced or erased during conversion, we must report an error because
+  // the newly inserted replacement op would have no parent context, and the
+  // original op cannot be properly unlinked.
+  SmallPtrSet<Operation *, 4> detachedRootOps;
+  for (Operation *op : ops)
+    if (!op->getBlock())
+      detachedRootOps.insert(op);
+
   LogicalResult status = legalizeOperations(ops, /*onFailure=*/[&]() {
     // Dialect conversion failed.
     if (rewriterImpl.config.allowPatternRollback) {
@@ -3473,6 +3487,21 @@ LogicalResult OperationConverter::applyConversion(ArrayRef<Operation *> ops) {
   if (failed(status))
     return failure();
 
+  // Check whether any originally-detached root op was replaced or erased
+  // during conversion. Such ops cannot be properly handled: the replacement op
+  // would be detached as well, and the original op cannot be unlinked from a
+  // (non-existent) parent block. Roll back and report an error.
+  if (rewriterImpl.config.allowPatternRollback) {
+    for (Operation *op : detachedRootOps) {
+      if (rewriterImpl.wasOpReplaced(op)) {
+        rewriterImpl.undoRewrites();
+        return op->emitError("dialect conversion requires that the root "
+                             "operation is nested within a block when it is "
+                             "replaced or erased by the conversion");
+      }
+    }
+  }
+
   // After a successful conversion, apply rewrites.
   rewriterImpl.applyRewrites();
 
diff --git a/mlir/test/Conversion/FuncToSPIRV/func-to-spirv-no-implicit-module.mlir b/mlir/test/Conversion/FuncToSPIRV/func-to-spirv-no-implicit-module.mlir
new file mode 100644
index 0000000000000..780c0997f3783
--- /dev/null
+++ b/mlir/test/Conversion/FuncToSPIRV/func-to-spirv-no-implicit-module.mlir
@@ -0,0 +1,9 @@
+// RUN: mlir-opt --no-implicit-module --convert-func-to-spirv -verify-diagnostics %s
+
+// Verify that converting a top-level func.func op via --no-implicit-module
+// fails gracefully without crashing. The dialect conversion infrastructure
+// must detect that the root operation has no parent block and emit an error.
+// Regression test for https://github.com/llvm/llvm-project/issues/60491
+
+// expected-error at below {{dialect conversion requires that the root operation is nested within a block when it is replaced or erased by the conversion}}
+func.func private @foo()

``````````

</details>


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