[mlir][emitc] Fix form-expressions inside expression

[kchibisov]

Make form-expressions not create emitc.expressions for operations inside the emitc.expressions, since they are invalid.

--

I've discovered it when I wanted to run this pass after some other transformation I perform, so I don't have to re-implement it inside my own pass, however given that I have emitc.expression inside the input, it was producing invalid IR.

Not sure whether it's the right way to fix it.

[llvmbot]

@llvm/pr-subscribers-mlir-emitc

@llvm/pr-subscribers-mlir

Author: Kirill Chibisov (kchibisov)

Changes

Make form-expressions not create emitc.expressions for operations inside the emitc.expressions, since they are invalid.

--

I've discovered it when I wanted to run this pass after some other transformation I perform, so I don't have to re-implement it inside my own pass, however given that I have emitc.expression inside the input, it was producing invalid IR.

Not sure whether it's the right way to fix it.

---

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

2 Files Affected:

• (modified) mlir/lib/Dialect/EmitC/Transforms/FormExpressions.cpp (+2-1)
• (modified) mlir/test/Dialect/EmitC/transforms.mlir (+17)

diff --git a/mlir/lib/Dialect/EmitC/Transforms/FormExpressions.cpp b/mlir/lib/Dialect/EmitC/Transforms/FormExpressions.cpp
index 5b03f81b305fd5..e7c431f39e3f08 100644
--- a/mlir/lib/Dialect/EmitC/Transforms/FormExpressions.cpp
+++ b/mlir/lib/Dialect/EmitC/Transforms/FormExpressions.cpp
@@ -36,7 +36,8 @@ struct FormExpressionsPass
     // Wrap each C operator op with an expression op.
     OpBuilder builder(context);
     auto matchFun = [&](Operation *op) {
-      if (op->hasTrait<OpTrait::emitc::CExpression>())
+      if (op->hasTrait<OpTrait::emitc::CExpression>() &&
+          !op->getParentOfType<emitc::ExpressionOp>())
         createExpression(op, builder);
     };
     rootOp->walk(matchFun);
diff --git a/mlir/test/Dialect/EmitC/transforms.mlir b/mlir/test/Dialect/EmitC/transforms.mlir
index ad167fa455a1a5..30551f09fd4da8 100644
--- a/mlir/test/Dialect/EmitC/transforms.mlir
+++ b/mlir/test/Dialect/EmitC/transforms.mlir
@@ -107,3 +107,20 @@ func.func @expression_with_address_taken(%arg0: i32, %arg1: i32, %arg2: !emitc.p
   %d = emitc.cmp lt, %c, %arg2 :(!emitc.ptr<i32>, !emitc.ptr<i32>) -> i1
   return %d : i1
 }
+
+// CHECK-LABEL: func.func @no_expression_in_expression(
+// CHECK-SAME:      %[[VAL_0:.*]]: i32, %[[VAL_1:.*]]: i32) -> i1 {
+// CHECK:         %[[VAL_2:.*]] = emitc.expression : i1 {
+// CHECK:           %[[VAL_3:.*]] = emitc.cmp lt, %[[VAL_0]], %[[VAL_1]] : (i32, i32) -> i1
+// CHECK:           emitc.yield %[[VAL_3]] : i1
+// CHECK:         }
+// CHECK:         return %[[VAL_2]] : i1
+// CHECK:       }
+
+func.func @no_expression_in_expression(%arg0: i32, %arg1: i32) -> i1 {
+  %a = emitc.expression : i1 {
+    %b = emitc.cmp lt, %arg0, %arg1 :(i32, i32) -> i1
+    emitc.yield %b : i1
+  }
+  return %a : i1
+}

[kchibisov]

CI failure looks unrelated.

[kchibisov]

Seems like CI fixed itself.

[marbre]

Thanks for fixing this! Just a minor suggestion regarding the name of the test.

[simon-camp]

Thanks, looks good to me.

[marbre]

Make form-expressions not create emitc.expressions for operations inside the emitc.expressions, since they are invalid.

--

I've discovered it when I wanted to run this pass after some other transformation I perform, so I don't have to re-implement it inside my own pass, however given that I have emitc.expression inside the input, it was producing invalid IR.

Not sure whether it's the right way to fix it.

Before commiting on your behalf, are you okay with using everything above -- for the commit message? GitHub proposes to use the entire PR description as squash commit message but I assume the first sentence is the better fit here.

[kchibisov]

-- as in git-send-email workflow, so everything below is a personal note for reviewer, which doesn't belong to commit.

I try to have commit messages correct/make sense, though you can also extend/reword if you feel like it.

[marbre]

-- as in git-send-email workflow, so everything below is a personal note for reviewer, which doesn't belong to commit.

Ah, I am not familiar with that tool :) But makes sense. I normally only add what I want in my squash commit message to the PR description. For everything else, I add an additional comment. But that's due to the GitHub workflow...

I try to have commit messages correct/make sense, though you can also extend/reword if you feel like it.

No need to, your explanation makes sense and also providing an additional note is perfectly fine.

[kchibisov]

Ah, I am not familiar with that tool :) B

I got it a bit wrong, it's ---, but you can see it in git format-patch as well, usually you write there arbitrary comments, they are all discarded by git when applying.

[aniragil]

Make form-expressions not create emitc.expressions for operations inside the emitc.expressions, since they are invalid.

Thanks for fixing this @kchibisov !