[mlir][scf] Add no_inline attribute to scf.execute_region

[jungpark-mlir]

Enabling users to explicitly specify which regions should be preserved, uncovers additional opportunities to utilize scf.execute_region op.

[llvmbot]

@llvm/pr-subscribers-mlir-scf

@llvm/pr-subscribers-mlir

Author: Jungwook Park (jungpark-mlir)

Changes

More control over the IR.
Enabling users to explicitly specify which regions should be preserved, uncovers additional opportunities to utilize scf.execute_region op.

---

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

3 Files Affected:

• (modified) mlir/include/mlir/Dialect/SCF/IR/SCFOps.td (+4)
• (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (+1-1)
• (modified) mlir/test/Dialect/SCF/canonicalize.mlir (+22)

diff --git a/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td b/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
index 2d15544e871b3..e6b83fca771a9 100644
--- a/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
+++ b/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
@@ -119,6 +119,10 @@ def ExecuteRegionOp : SCF_Op<"execute_region", [
     ```
   }];
 
+  let arguments = (ins
+    UnitAttr:$no_inline
+  );
+
   let results = (outs Variadic<AnyType>);
 
   let regions = (region AnyRegion:$region);
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 759e58b617578..6b7f11a3469d6 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -184,7 +184,7 @@ struct SingleBlockExecuteInliner : public OpRewritePattern<ExecuteRegionOp> {
 
   LogicalResult matchAndRewrite(ExecuteRegionOp op,
                                 PatternRewriter &rewriter) const override {
-    if (!op.getRegion().hasOneBlock())
+    if (!op.getRegion().hasOneBlock() || op.getNoInline())
       return failure();
     replaceOpWithRegion(rewriter, op, op.getRegion());
     return success();
diff --git a/mlir/test/Dialect/SCF/canonicalize.mlir b/mlir/test/Dialect/SCF/canonicalize.mlir
index 12d30e17f4a8f..ae02144f06f32 100644
--- a/mlir/test/Dialect/SCF/canonicalize.mlir
+++ b/mlir/test/Dialect/SCF/canonicalize.mlir
@@ -1461,6 +1461,28 @@ func.func @execute_region_elim() {
 
 // -----
 
+// CHECK-LABEL: func @execute_region_elim_noinline
+func.func @execute_region_elim_noinline() {
+  affine.for %i = 0 to 100 {
+    "test.foo"() : () -> ()
+    %v = scf.execute_region -> i64 {
+      %x = "test.val"() : () -> i64
+      scf.yield %x : i64
+    } {no_inline}
+    "test.bar"(%v) : (i64) -> ()
+  }
+  return
+}
+
+// CHECK-NEXT:     affine.for %arg0 = 0 to 100 {
+// CHECK-NEXT:       "test.foo"() : () -> ()
+// CHECK-NEXT:       scf.execute_region
+// CHECK-NEXT:       %[[VAL:.*]] = "test.val"() : () -> i64
+// CHECK-NEXT:       scf.yield %[[VAL]] : i64
+// CHECK-NEXT:     }
+
+// -----
+
 // CHECK-LABEL: func @func_execute_region_elim
 func.func @func_execute_region_elim() {
     "test.foo"() : () -> ()

[joker-eph]

Please handle this in the assembly format directly as a property instead of letting this part of the attribute dictionary.

Also I would rather see the keyword on the first line.

[matthias-springer]

Can you also add a sentence to the description?

[jungpark-mlir]

Please handle this in the assembly format directly as a property instead of letting this part of the attribute dictionary.

@joker-eph Please see if I understand this correctly,
scf.execute_region uses custom assembly and it already parse/print optional attributes, so only need to document in the description?
And use op->getAttr("no_inline") and setAttr when needed?

[joker-eph]

Actually you're missing the distinction between inherent and discardable attributes. We can already have discardable attributes, but here you're adding an inherent one by declaring it in ODS, so I wouldn't want it to be part of the discardable ones in the syntax.
So I'd like the actual printing/parsing code to parse an optional keyword "no_inline" on the first line before the body.

[jungpark-mlir]

Actually you're missing the distinction between inherent and discardable attributes. We can already have discardable attributes, but here you're adding an inherent one by declaring it in ODS, so I wouldn't want it to be part of the discardable ones in the syntax. So I'd like the actual printing/parsing code to parse an optional keyword "no_inline" on the first line before the body.

Got it!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[joker-eph]

It's a bit weird to describe an op semantics in terms of a specific pass.

Suggested change

	Canonicalizer inlines an ExecuteRegionOp into its parent if it only contains
	one block or its parent can contain multiple blocks, 'no_inline' attribute
	can be set to prevent an ExecuteRegionOp from being inlined.
	The optional 'no_inline' flag can be set to request the ExecuteRegionOp to be
	preserved as much as possible and not being inlined in the parent block until
	an explicit lowering step.

[jungpark-mlir]

agreed, thanks!

[joker-eph]

Thanks.