[mlir][emitc] mark `emitc.load` with `CExpression` #130802

kchibisov · 2025-03-11T17:10:29Z

Follow the call and call_opaque operations, as well as apply, which already are marked as CExpression even though they have side effects.

Even though emitc.load can be included inside the emitc.expression, the inlining and --form-expression pass won't actually inline them inside other expression due to it having a side effect, thus unless the user manually writes the emitc.load inside the emitc.expression it won't appear there.

--

It was brought #91475 (comment) and while there was some opposition due to load having a side effect, emitc already allows all the rest operations that have it, so for consistency reasons, enabling it doesn't really hurt from my point of view. Especially given that --form-expression doesn't allow
it to really inline inside other expressions, which makes sense, since if the users want such behavior, they should explicitly opt-in.

llvmbot · 2025-03-11T17:11:02Z

@llvm/pr-subscribers-mlir-emitc

@llvm/pr-subscribers-mlir

Author: Kirill Chibisov (kchibisov)

Changes

Follow the call and call_opaque operations, as well as apply, which already are marked as CExpression even though they have side effects.

Even though emitc.load can be included inside the emitc.expression, the inlining and --form-expression pass won't actually inline them inside other expression due to it having a side effect, thus unless the user manually writes the emitc.load inside the emitc.expression it won't appear there.

--

It was brought #91475 (comment) and while there was some opposition due to load having a side effect, emitc already allows all the rest operations that have it, so for consistency reasons, enabling it doesn't really hurt from my point of view. Especially given that --form-expression doesn't allow
it to really inline inside other expressions, which makes sense, since if the users want such behavior, they should explicitly opt-in.

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

3 Files Affected:

(modified) mlir/include/mlir/Dialect/EmitC/IR/EmitC.td (+1-1)
(modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+1)
(modified) mlir/test/Dialect/EmitC/transforms.mlir (+23)

diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index a518e7425956c..af2eed81e0d5b 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -927,7 +927,7 @@ def EmitC_LogicalOrOp : EmitC_BinaryOp<"logical_or", [CExpression]> {
   let assemblyFormat = "operands attr-dict `:` type(operands)";
 }
 
-def EmitC_LoadOp : EmitC_Op<"load", [
+def EmitC_LoadOp : EmitC_Op<"load", [CExpression,
   TypesMatchWith<"result type matches value type of 'operand'",
                   "operand", "result",
                   "::llvm::cast<LValueType>($_self).getValueType()">
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index b00820ffc542b..a3761ead4adde 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -100,6 +100,7 @@ static FailureOr<int> getOperatorPrecedence(Operation *operation) {
       })
       .Case<emitc::ConditionalOp>([&](auto op) { return 2; })
       .Case<emitc::DivOp>([&](auto op) { return 13; })
+      .Case<emitc::LoadOp>([&](auto op) { return 16; })
       .Case<emitc::LogicalAndOp>([&](auto op) { return 4; })
       .Case<emitc::LogicalNotOp>([&](auto op) { return 15; })
       .Case<emitc::LogicalOrOp>([&](auto op) { return 3; })
diff --git a/mlir/test/Dialect/EmitC/transforms.mlir b/mlir/test/Dialect/EmitC/transforms.mlir
index d204dec70d449..0678b4e5515c0 100644
--- a/mlir/test/Dialect/EmitC/transforms.mlir
+++ b/mlir/test/Dialect/EmitC/transforms.mlir
@@ -129,3 +129,26 @@ func.func @single_result_requirement() -> (i32, i32) {
   %0:2 = emitc.call_opaque "foo" () : () -> (i32, i32)
   return %0#0, %0#1 : i32, i32
 }
+
+// CHECK-LABEL: func.func @expression_with_load(
+// CHECK-SAME:      %[[VAL_0:.*]]: i32, %[[VAL_1:.*]]: i32) -> i1 {
+// CHECK:         %[[VAL_2:.*]] = "emitc.variable"() <{value = #emitc.opaque<"42">}> : () -> !emitc.lvalue<i32>
+// CHECK:         %[[VAL_3:.*]] = emitc.expression : i32 {
+// CHECK:           %[[VAL_4:.*]] = load %[[VAL_2]] : <i32>
+// CHECK:           yield %[[VAL_4]] : i32
+// CHECK:         }
+// CHECK:         %[[VAL_5:.*]] = emitc.expression : i1 {
+// CHECK:           %[[VAL_6:.*]] = add %[[VAL_3]], %[[VAL_1]] : (i32, i32) -> i32
+// CHECK:           %[[VAL_7:.*]] = cmp lt, %[[VAL_6]], %[[VAL_0]] : (i32, i32) -> i1
+// CHECK:           yield %[[VAL_7]] : i1
+// CHECK:         }
+// CHECK:         return %[[VAL_5]] : i1
+// CHECK:       }
+
+func.func @expression_with_load(%arg0: i32, %arg1: i32) -> i1 {
+  %0 = "emitc.variable"() <{value = #emitc.opaque<"42">}> : () -> !emitc.lvalue<i32>
+  %a = emitc.load %0 : !emitc.lvalue<i32>
+  %b = emitc.add %a, %arg1 : (i32, i32) -> i32
+  %c = emitc.cmp lt, %b, %arg0 :(i32, i32) -> i1
+  return %c : i1
+}

mgehre-amd · 2025-03-12T10:01:20Z

Can you please add some tests for the output of translation? Interesting test case could also be loading from an array index, and then either having the emitc.subscript also within the CExpression or outside.

simon-camp · 2025-03-12T13:55:59Z

Thanks for addressing this.

Can you please add some tests for the output of translation? Interesting test case could also be loading from an array index, and then either having the emitc.subscript also within the CExpression or outside.

On top of that a translation test with a combination of a load inside an expression and a call_opaque op would be useful. I'm not sure if the precedence shouldn't be higher than that of any other op.

kchibisov · 2025-03-13T12:23:26Z

Can you please add some tests for the output of translation? Interesting test case could also be loading from an array index,

Sure done.

and then either having the emitc.subscript also within the CExpression or outside

emitc.subscript is not a CExpression.

On top of that a translation test with a combination of a load inside an expression and a call_opaque op would be useful. I'm not sure if the precedence shouldn't be higher than that of any other op.

Sure, done. If it's not what you've expected, let me know.

mgehre-amd

Looks good to me. Let's wait for @simon-camp to say whether the test for precedence is what he expected.

simon-camp

LGTM, thanks.

kchibisov · 2025-03-13T14:43:10Z

I've just noticed one inconsistency in the handling of CExpression inlining. If you look at hasSideEffects impl it does prevent uses of call_opaque and e.g. variable to not result in inlining, however, not e.g. subscript Op and regular call.

In general, variable right now is assumed to read local storage variable and can not really load from memory that may result in undefined behavior. Thus, I think the hasSideEffects should probably disallow subscript as it has side effect, and allow variable as loading from it doesn't have a side effect.

As for the call, I'm not quite sure how to deal with it and maybe keep as is or disallow all together.

Edit:

This also applies to member, but IIRC it acts as variable, but not member_of_ptr. There's also get_global, etc.

Follow the `call` and `call_opaque` operations, as well as `apply`, which already are marked as `CExpression` even though they have side effects. Even though `emitc.load` can be included inside the `emitc.expression`, the inlining and `--form-expression` pass won't actually inline them inside other expression due to it having a side effect, thus unless the user manually writes the `emitc.load` inside the `emitc.expression` it won't appear there. Also update the `hasSideEffects` to account for `LoadOp` instead of `VariableOp`. The `VariableOp` was used before the `LoadOp` was introduced, but at the present time, the `LoadOp` is what is being used to actually load and emit the instruction that does the loading.

kchibisov · 2025-03-13T15:31:56Z

@mgehre-amd @simon-camp I've updated PR to make a correct hasSideEffects check, since it does matter now. The old one was a left over from the time where Variable was the only way to define something that will actually have a loading.

The loosening of what kind of load is considered a side effect I'll do separately. The same with e.g. call if we want that.

simon-camp · 2025-03-14T07:54:50Z

I don't see theses changes, maybe you forgot to push your changes?

kchibisov · 2025-03-14T11:04:15Z

Indeed, sorry. Will re-ping once I'll be able to push changes.

kchibisov · 2025-03-14T13:48:43Z

@simon-camp should be pushed now.

simon-camp

Thanks.

I wonder if we should upgrade the CExpression trait to an interface, so that the ops themselves can override a method. Another option could be to use MLIR MemoryEffects instead.

kchibisov · 2025-03-20T08:21:00Z

I wonder if we should upgrade the CExpression trait to an interface, so that the ops themselves can override a method. Another option could be to use MLIR MemoryEffects instead.

I'll look into this for e.g. lowering the restrictions PR, but for this one, I think it's fine as is, since it was more of a bug fix.

@mgehre-amd anything else needed from my side?

kchibisov · 2025-04-04T10:53:53Z

ping

aniragil

Sorry for the delay, this somehow slipped under my radar.
LGTM, thanks for handling this!

kchibisov · 2025-04-21T08:55:35Z

ping

marbre · 2025-04-22T06:21:02Z

ping

If you don't have write access, I am happy to merge this for you, it that is what you're requesting.

kchibisov · 2025-04-22T06:26:01Z

Yes, I don't have write access, so I need someone to merge.

llvmbot added mlir mlir:emitc labels Mar 11, 2025

marbre requested review from simon-camp, aniragil and mgehre-amd March 12, 2025 09:55

kchibisov force-pushed the emitc-load-op branch from 20b3873 to 4d23b0d Compare March 13, 2025 12:21

mgehre-amd approved these changes Mar 13, 2025

View reviewed changes

simon-camp approved these changes Mar 13, 2025

View reviewed changes

kchibisov requested review from mgehre-amd and simon-camp March 13, 2025 15:29

kchibisov force-pushed the emitc-load-op branch from 4d23b0d to 6b8d99d Compare March 14, 2025 13:47

simon-camp approved these changes Mar 18, 2025

View reviewed changes

aniragil approved these changes Apr 5, 2025

View reviewed changes

marbre merged commit 0ca2d4d into llvm:main Apr 22, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir][emitc] mark `emitc.load` with `CExpression` #130802

[mlir][emitc] mark `emitc.load` with `CExpression` #130802

kchibisov commented Mar 11, 2025

llvmbot commented Mar 11, 2025 •

edited

Loading

mgehre-amd commented Mar 12, 2025

simon-camp commented Mar 12, 2025

kchibisov commented Mar 13, 2025

mgehre-amd left a comment

simon-camp left a comment

kchibisov commented Mar 13, 2025 •

edited

Loading

kchibisov commented Mar 13, 2025

simon-camp commented Mar 14, 2025

kchibisov commented Mar 14, 2025

kchibisov commented Mar 14, 2025 •

edited

Loading

simon-camp left a comment

kchibisov commented Mar 20, 2025

kchibisov commented Apr 4, 2025

aniragil left a comment

kchibisov commented Apr 21, 2025

marbre commented Apr 22, 2025

kchibisov commented Apr 22, 2025

[mlir][emitc] mark emitc.load with CExpression #130802

[mlir][emitc] mark emitc.load with CExpression #130802

Conversation

kchibisov commented Mar 11, 2025

llvmbot commented Mar 11, 2025 • edited Loading

mgehre-amd commented Mar 12, 2025

simon-camp commented Mar 12, 2025

kchibisov commented Mar 13, 2025

mgehre-amd left a comment

Choose a reason for hiding this comment

simon-camp left a comment

Choose a reason for hiding this comment

kchibisov commented Mar 13, 2025 • edited Loading

kchibisov commented Mar 13, 2025

simon-camp commented Mar 14, 2025

kchibisov commented Mar 14, 2025

kchibisov commented Mar 14, 2025 • edited Loading

simon-camp left a comment

Choose a reason for hiding this comment

kchibisov commented Mar 20, 2025

kchibisov commented Apr 4, 2025

aniragil left a comment

Choose a reason for hiding this comment

kchibisov commented Apr 21, 2025

marbre commented Apr 22, 2025

kchibisov commented Apr 22, 2025

[mlir][emitc] mark `emitc.load` with `CExpression` #130802

[mlir][emitc] mark `emitc.load` with `CExpression` #130802

llvmbot commented Mar 11, 2025 •

edited

Loading

kchibisov commented Mar 13, 2025 •

edited

Loading

kchibisov commented Mar 14, 2025 •

edited

Loading