[MLIR][ControlFlowToSCF] Extend with support for FuncOpInterface

[Dinistro]

This commit ensures that the pass to lift from cf to scf can be applied on all operations that implement FuncOpInterface.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Christian Ulmann (Dinistro)

Changes

This commit ensures that the pass to lift from cf to scf can be applied on all operations that implement FuncOpInterface.

---

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

2 Files Affected:

• (modified) mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp (+2-2)
• (modified) mlir/test/Conversion/ControlFlowToSCF/test.mlir (+26)

diff --git a/mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp b/mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp
index 1c592d665f3e4c..a54281dfd33753 100644
--- a/mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp
+++ b/mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp
@@ -164,8 +164,8 @@ struct LiftControlFlowToSCF
 
     bool changed = false;
     Operation *op = getOperation();
-    WalkResult result = op->walk([&](func::FuncOp funcOp) {
-      if (funcOp.getBody().empty())
+    WalkResult result = op->walk([&](FunctionOpInterface funcOp) {
+      if (funcOp.getFunctionBody().empty())
         return WalkResult::advance();
 
       auto &domInfo = funcOp != op ? getChildAnalysis<DominanceInfo>(funcOp)
diff --git a/mlir/test/Conversion/ControlFlowToSCF/test.mlir b/mlir/test/Conversion/ControlFlowToSCF/test.mlir
index f6e7fb265790b7..3a2ec521aae0a0 100644
--- a/mlir/test/Conversion/ControlFlowToSCF/test.mlir
+++ b/mlir/test/Conversion/ControlFlowToSCF/test.mlir
@@ -756,3 +756,29 @@ func.func @nested_region_outside_loop_use() {
 
 // CHECK: scf.execute_region
 // CHECK-NEXT: "test.foo"(%[[RES]])
+
+// -----
+
+llvm.func @llvm_func() {
+  %cond = "test.test1"() : () -> i1
+  cf.cond_br %cond, ^bb1, ^bb2
+^bb1:
+  "test.test2"() : () -> ()
+  cf.br ^bb3
+^bb2:
+  "test.test3"() : () -> ()
+  cf.br ^bb3
+^bb3:
+  "test.test4"() : () -> ()
+  return
+}
+
+// CHECK-LABEL: llvm.func @llvm_func
+// CHECK:      %[[COND:.*]] = "test.test1"()
+// CHECK-NEXT: scf.if %[[COND]]
+// CHECK-NEXT:   "test.test2"()
+// CHECK-NEXT: else
+// CHECK-NEXT:   "test.test3"()
+// CHECK-NEXT: }
+// CHECK-NEXT: "test.test4"()
+// CHECK-NEXT: return

[zero9178]

Making this work using the function interface makes sense!

The only problem I see is further up at line 147. It very much assumes it is nested in a func::FuncOp. We'd need either some other logic to create the right kind of return like operation, or the ub.unreachable mentioned in the TODO.

Otherwise code such as:

llvm.func @foo() {
  cf.br

^bb0:
  llvm.call @sideeffect()
  cf.br ^bb0
}

will cause an error

[Dinistro]

I totally forgot about this. I fear that adding something that checks on the function type is not really desired, while implementing a ub.unreachable sounds a bit controversial. Adding support for such a new operation in all kinds of verifiers might be tricky.

[Dinistro]

Closing this for now, as underlying work will be necessary for this to eventually work.