[mlir] fix crash when scf utils work on llvm.func

[cxy-1993]

fixed #119378

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-scf

Author: donald chen (cxy-1993)

Changes

fixed #119378

---

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

3 Files Affected:

• (modified) mlir/lib/Dialect/SCF/Utils/Utils.cpp (+1-1)
• (modified) mlir/test/Transforms/scf-if-utils.mlir (+12)
• (modified) mlir/test/lib/Dialect/SCF/TestSCFUtils.cpp (+4)

diff --git a/mlir/lib/Dialect/SCF/Utils/Utils.cpp b/mlir/lib/Dialect/SCF/Utils/Utils.cpp
index e341c3744f1d8f..41410a0a56aa98 100644
--- a/mlir/lib/Dialect/SCF/Utils/Utils.cpp
+++ b/mlir/lib/Dialect/SCF/Utils/Utils.cpp
@@ -130,7 +130,7 @@ FailureOr<func::FuncOp> mlir::outlineSingleBlockRegion(RewriterBase &rewriter,
 
   // Outline before current function.
   OpBuilder::InsertionGuard g(rewriter);
-  rewriter.setInsertionPoint(region.getParentOfType<func::FuncOp>());
+  rewriter.setInsertionPoint(region.getParentOfType<FunctionOpInterface>());
 
   SetVector<Value> captures;
   getUsedValuesDefinedAbove(region, captures);
diff --git a/mlir/test/Transforms/scf-if-utils.mlir b/mlir/test/Transforms/scf-if-utils.mlir
index 449be18483741c..2825ff19bfd018 100644
--- a/mlir/test/Transforms/scf-if-utils.mlir
+++ b/mlir/test/Transforms/scf-if-utils.mlir
@@ -73,3 +73,15 @@ func.func @outline_empty_if_else(%cond: i1, %a: index, %b: memref<?xf32>, %c: i8
   }
   return
 }
+
+// -----
+
+// This test checks scf utils can work on llvm func.
+// CHECK-LABEL: @llvm_func
+llvm.func @llvm_func(%cond: i1, %a: i32) {
+  scf.if %cond {
+  } else {
+    "some_op"(%cond, %a) : (i1, i32) -> ()
+  }
+  llvm.return
+}
diff --git a/mlir/test/lib/Dialect/SCF/TestSCFUtils.cpp b/mlir/test/lib/Dialect/SCF/TestSCFUtils.cpp
index 3ff7f9966e93da..a3be1f94fa28a3 100644
--- a/mlir/test/lib/Dialect/SCF/TestSCFUtils.cpp
+++ b/mlir/test/lib/Dialect/SCF/TestSCFUtils.cpp
@@ -79,6 +79,10 @@ struct TestSCFIfUtilsPass
   StringRef getDescription() const final { return "test scf.if utils"; }
   explicit TestSCFIfUtilsPass() = default;
 
+  void getDependentDialects(DialectRegistry &registry) const override {
+    registry.insert<func::FuncDialect>();
+  }
+
   void runOnOperation() override {
     int count = 0;
     getOperation().walk([&](scf::IfOp ifOp) {

[ftynse]

I edited the title of the PR, because a bug fix is certainly NOT a non-functional change (nfc).

[ftynse]

It's feels odd to me to create func.func when outlining from llvm.func, but I suppose we can live with that. The better solution would be to find a way to create a function, and a call to it, of the same kind as the current function, but it is likely impossible without modifications to the function interface. So we can live with what's proposed here + additional lowering.

Please make sure the test actually tests the change.

[cxy-1993]

It's feels odd to me to create func.func when outlining from llvm.func, but I suppose we can live with that. The better solution would be to find a way to create a function, and a call to it, of the same kind as the current function, but it is likely impossible without modifications to the function interface. So we can live with what's proposed here + additional lowering.

Please make sure the test actually tests the change.

Thanks for the comments on this patch. I agree it would be better if we could make a matching function op, but it's hard to do right now.

Regarding your comment on test, it was primarily designed to prevent crashes under llvm func. However, following your suggestion, I've also included checks for outlining.