[mlir][emitc] Turn constant into CExpression #156450
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This patch follows the same motivation behind this patch, but aims to do so via the |
aniragil
requested review from
jacquesguan,
marbre,
simon-camp and
mgehre-amd
The `emitc.constant` op was so far left out of `emit.expression`'s as its ConstantLike trait could cause CSE to invalidate `emitc.expression` ops in two ways: - Remove the root of a constant-only expression, leaving the expression empty. - Simplify within the expression, violating the single-use requirement. The first issue was recently resolved by making `emitc.expression` isolated-from-above. The second is resolved here by limiting the single-use requirement to CExpressions with side effects, as ops with no side effects can safely be cloned as needed.
aniragil
force-pushed
the
emitc-constant-as-expression
branch
from
5cc33b8 to
20cbb69
Compare
marbre
approved these changes
Sep 3, 2025
Oh, right: The Actually, the entire single-use restriction can arguably be dropped: As the semantics of |
|
@llvm/pr-subscribers-mlir Author: Gil Rapaport (aniragil) ChangesThe
The first issue was recently resolved by making Full diff: https://github.com/llvm/llvm-project/pull/156450.diff 7 Files Affected:
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index fb7a108b39fc0..721f9f6b320ad 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -402,7 +402,8 @@ def EmitC_CmpOp : EmitC_BinaryOp<"cmp", []> {
let assemblyFormat = "$predicate `,` operands attr-dict `:` functional-type(operands, results)";
}
-def EmitC_ConstantOp : EmitC_Op<"constant", [ConstantLike]> {
+def EmitC_ConstantOp
+ : EmitC_Op<"constant", [ConstantLike, CExpressionInterface]> {
let summary = "Constant operation";
let description = [{
The `emitc.constant` operation produces an SSA value equal to some constant
@@ -429,6 +430,13 @@ def EmitC_ConstantOp : EmitC_Op<"constant", [ConstantLike]> {
let hasFolder = 1;
let hasVerifier = 1;
+
+ let extraClassDeclaration = [{
+ bool hasSideEffects() {
+ // If operand is fundamental type, the operation is pure.
+ return !isFundamentalType(getResult().getType());
+ }
+ }];
}
def EmitC_DivOp : EmitC_BinaryOp<"div", []> {
@@ -466,8 +474,9 @@ def EmitC_ExpressionOp
its single-basic-block region. The operation doesn't take any arguments.
As the operation is to be emitted as a C expression, the operations within
- its body must form a single Def-Use tree of emitc ops whose result is
- yielded by a terminating `emitc.yield`.
+ its body must form a single Def-Use tree, or a DAG trivially expandable to
+ one, i.e. a DAG where each operation with side effects is only reachable
+ once from the expression root.
Example:
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 00ce3b59bf870..a73470cdf76c5 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -14,7 +14,9 @@
#include "mlir/IR/DialectImplementation.h"
#include "mlir/IR/Types.h"
#include "mlir/Interfaces/FunctionImplementation.h"
+#include "mlir/Support/LLVM.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/TypeSwitch.h"
#include "llvm/Support/Casting.h"
@@ -462,12 +464,34 @@ LogicalResult ExpressionOp::verify() {
return emitOpError("requires yielded type to match return type");
for (Operation &op : region.front().without_terminator()) {
- if (!isa<emitc::CExpressionInterface>(op))
+ auto expressionInterface = dyn_cast<emitc::CExpressionInterface>(op);
+ if (!expressionInterface)
return emitOpError("contains an unsupported operation");
if (op.getNumResults() != 1)
return emitOpError("requires exactly one result for each operation");
- if (!op.getResult(0).hasOneUse())
- return emitOpError("requires exactly one use for each operation");
+ Value result = op.getResult(0);
+ if (result.use_empty())
+ return emitOpError("contains an unused operation");
+ }
+
+ // Make sure any operation with side effect is only reachable once from
+ // the root op, otherwise emission will be replicating side effects.
+ SmallPtrSet<Operation *, 16> visited;
+ SmallVector<Operation *> worklist;
+ worklist.push_back(rootOp);
+ while (!worklist.empty()) {
+ Operation *op = worklist.back();
+ worklist.pop_back();
+ if (visited.contains(op)) {
+ if (cast<CExpressionInterface>(op).hasSideEffects())
+ return emitOpError(
+ "requires exactly one use for operations with side effects");
+ }
+ visited.insert(op);
+ for (Value operand : op->getOperands())
+ if (Operation *def = operand.getDefiningOp()) {
+ worklist.push_back(def);
+ }
}
return success();
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 570f38c60020b..a5bd80e9d6b8b 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -97,6 +97,7 @@ static FailureOr<int> getOperatorPrecedence(Operation *operation) {
return op->emitError("unsupported cmp predicate");
})
.Case<emitc::ConditionalOp>([&](auto op) { return 2; })
+ .Case<emitc::ConstantOp>([&](auto op) { return 17; })
.Case<emitc::DivOp>([&](auto op) { return 13; })
.Case<emitc::LoadOp>([&](auto op) { return 16; })
.Case<emitc::LogicalAndOp>([&](auto op) { return 4; })
@@ -267,8 +268,14 @@ struct CppEmitter {
Operation *def = value.getDefiningOp();
if (!def)
return false;
+ return isPartOfCurrentExpression(def);
+ }
+
+ /// Determine whether given operation is part of the expression potentially
+ /// being emitted.
+ bool isPartOfCurrentExpression(Operation *def) {
auto operandExpression = dyn_cast<ExpressionOp>(def->getParentOp());
- return operandExpression == emittedExpression;
+ return operandExpression && operandExpression == emittedExpression;
};
// Resets the value counter to 0.
@@ -408,6 +415,9 @@ static LogicalResult printOperation(CppEmitter &emitter,
Operation *operation = constantOp.getOperation();
Attribute value = constantOp.getValue();
+ if (emitter.isPartOfCurrentExpression(operation))
+ return emitter.emitAttribute(operation->getLoc(), value);
+
return printConstantOp(emitter, operation, value);
}
diff --git a/mlir/test/Dialect/EmitC/form-expressions.mlir b/mlir/test/Dialect/EmitC/form-expressions.mlir
index 67cd6fddba638..7b6723989e260 100644
--- a/mlir/test/Dialect/EmitC/form-expressions.mlir
+++ b/mlir/test/Dialect/EmitC/form-expressions.mlir
@@ -2,8 +2,8 @@
// CHECK-LABEL: func.func @single_expression(
// CHECK-SAME: %[[VAL_0:.*]]: i32, %[[VAL_1:.*]]: i32, %[[VAL_2:.*]]: i32, %[[VAL_3:.*]]: i32) -> i1 {
-// CHECK: %[[VAL_4:.*]] = "emitc.constant"() <{value = 42 : i32}> : () -> i32
-// CHECK: %[[VAL_5:.*]] = emitc.expression %[[VAL_3]], %[[VAL_2]], %[[VAL_0]], %[[VAL_4]] : (i32, i32, i32, i32) -> i1 {
+// CHECK: %[[VAL_5:.*]] = emitc.expression %[[VAL_3]], %[[VAL_2]], %[[VAL_0]] : (i32, i32, i32) -> i1 {
+// CHECK: %[[VAL_4:.*]] = "emitc.constant"() <{value = 42 : i32}> : () -> i32
// CHECK: %[[VAL_6:.*]] = mul %[[VAL_0]], %[[VAL_4]] : (i32, i32) -> i32
// CHECK: %[[VAL_7:.*]] = sub %[[VAL_6]], %[[VAL_2]] : (i32, i32) -> i32
// CHECK: %[[VAL_8:.*]] = cmp lt, %[[VAL_7]], %[[VAL_3]] : (i32, i32) -> i1
@@ -131,7 +131,10 @@ func.func @single_result_requirement() -> (i32, i32) {
// CHECK-LABEL: func.func @expression_with_load(
// CHECK-SAME: %[[VAL_0:.*]]: i32,
// CHECK-SAME: %[[VAL_1:.*]]: !emitc.ptr<i32>) -> i1 {
-// CHECK: %[[VAL_2:.*]] = "emitc.constant"() <{value = 0 : i64}> : () -> i64
+// CHECK: %[[VAL_2:.*]] = emitc.expression : () -> i64 {
+// CHECK: %[[VAL_C:.*]] = "emitc.constant"() <{value = 0 : i64}> : () -> i64
+// CHECK: yield %[[VAL_C]] : i64
+// CHECK: }
// CHECK: %[[VAL_3:.*]] = "emitc.variable"() <{value = #emitc.opaque<"42">}> : () -> !emitc.lvalue<i32>
// CHECK: %[[VAL_4:.*]] = emitc.expression %[[VAL_3]] : (!emitc.lvalue<i32>) -> i32 {
// CHECK: %[[VAL_5:.*]] = load %[[VAL_3]] : <i32>
@@ -162,24 +165,46 @@ func.func @expression_with_load(%arg0: i32, %arg1: !emitc.ptr<i32>) -> i1 {
}
// CHECK-LABEL: func.func @opaque_type_expression(%arg0: i32, %arg1: !emitc.opaque<"T0">, %arg2: i32) -> i1 {
-// CHECK: %0 = "emitc.constant"() <{value = 42 : i32}> : () -> i32
-// CHECK: %1 = emitc.expression %arg1, %arg0, %0 : (!emitc.opaque<"T0">, i32, i32) -> i32 {
-// CHECK: %3 = mul %arg0, %0 : (i32, i32) -> i32
-// CHECK: %4 = sub %3, %arg1 : (i32, !emitc.opaque<"T0">) -> i32
+// CHECK: %0 = emitc.expression : () -> !emitc.opaque<"T1"> {
+// CHECK: %4 = "emitc.constant"() <{value = #emitc.opaque<"V">}> : () -> !emitc.opaque<"T1">
+// CHECK: yield %4 : !emitc.opaque<"T1">
+// CHECK: }
+// CHECK: %1 = emitc.expression %arg0, %0 : (i32, !emitc.opaque<"T1">) -> i32 {
+// CHECK: %4 = mul %arg0, %0 : (i32, !emitc.opaque<"T1">) -> i32
+// CHECK: yield %4 : i32
+// CHECK: }
+// CHECK: %2 = emitc.expression %1, %arg1 : (i32, !emitc.opaque<"T0">) -> i32 {
+// CHECK: %4 = sub %1, %arg1 : (i32, !emitc.opaque<"T0">) -> i32
// CHECK: yield %4 : i32
// CHECK: }
-// CHECK: %2 = emitc.expression %1, %arg2 : (i32, i32) -> i1 {
-// CHECK: %3 = cmp lt, %1, %arg2 : (i32, i32) -> i1
-// CHECK: yield %3 : i1
+// CHECK: %3 = emitc.expression %2, %arg2 : (i32, i32) -> i1 {
+// CHECK: %4 = cmp lt, %2, %arg2 : (i32, i32) -> i1
+// CHECK: yield %4 : i1
// CHECK: }
-// CHECK: return %2 : i1
+// CHECK: return %3 : i1
// CHECK: }
func.func @opaque_type_expression(%arg0: i32, %arg1: !emitc.opaque<"T0">, %arg2: i32) -> i1 {
- %c42 = "emitc.constant"(){value = 42 : i32} : () -> i32
- %a = emitc.mul %arg0, %c42 : (i32, i32) -> i32
+ %c42 = "emitc.constant"(){value = #emitc.opaque<"V">} : () -> !emitc.opaque<"T1">
+ %a = emitc.mul %arg0, %c42 : (i32, !emitc.opaque<"T1">) -> i32
%b = emitc.sub %a, %arg1 : (i32, !emitc.opaque<"T0">) -> i32
%c = emitc.cmp lt, %b, %arg2 :(i32, i32) -> i1
return %c : i1
}
+
+// CHECK-LABEL: func.func @expression_with_constant(
+// CHECK-SAME: %[[VAL_0:.*]]: i32) -> i32 {
+// CHECK: %[[VAL_1:.*]] = emitc.expression %[[VAL_0]] : (i32) -> i32 {
+// CHECK: %[[VAL_2:.*]] = "emitc.constant"() <{value = 42 : i32}> : () -> i32
+// CHECK: %[[VAL_3:.*]] = mul %[[VAL_0]], %[[VAL_2]] : (i32, i32) -> i32
+// CHECK: yield %[[VAL_3]] : i32
+// CHECK: }
+// CHECK: return %[[VAL_1]] : i32
+// CHECK: }
+
+func.func @expression_with_constant(%arg0: i32) -> i32 {
+ %c42 = "emitc.constant"(){value = 42 : i32} : () -> i32
+ %a = emitc.mul %arg0, %c42 : (i32, i32) -> i32
+ return %a : i32
+}
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index fdfb0eb46f7c5..a97474401645c 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -311,7 +311,7 @@ func.func @test_expression_illegal_op(%arg0 : i1) -> i32 {
// -----
func.func @test_expression_no_use(%arg0: i32, %arg1: i32) -> i32 {
- // expected-error @+1 {{'emitc.expression' op requires exactly one use for each operation}}
+ // expected-error @+1 {{'emitc.expression' op contains an unused operation}}
%r = emitc.expression %arg0, %arg1 : (i32, i32) -> i32 {
%a = emitc.add %arg0, %arg1 : (i32, i32) -> i32
%b = emitc.rem %arg0, %arg1 : (i32, i32) -> i32
@@ -323,12 +323,12 @@ func.func @test_expression_no_use(%arg0: i32, %arg1: i32) -> i32 {
// -----
func.func @test_expression_multiple_uses(%arg0: i32, %arg1: i32) -> i32 {
- // expected-error @+1 {{'emitc.expression' op requires exactly one use for each operation}}
+ // expected-error @+1 {{'emitc.expression' op requires exactly one use for operations with side effects}}
%r = emitc.expression %arg0, %arg1 : (i32, i32) -> i32 {
- %a = emitc.rem %arg0, %arg1 : (i32, i32) -> i32
+ %a = emitc.call_opaque "foo"(%arg0, %arg1) : (i32, i32) -> i32
%b = emitc.add %a, %arg0 : (i32, i32) -> i32
- %c = emitc.mul %arg1, %a : (i32, i32) -> i32
- emitc.yield %a : i32
+ %c = emitc.mul %b, %a : (i32, i32) -> i32
+ emitc.yield %c : i32
}
return %r : i32
}
diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir
index e890f77173de7..84c9b65d775d2 100644
--- a/mlir/test/Dialect/EmitC/ops.mlir
+++ b/mlir/test/Dialect/EmitC/ops.mlir
@@ -203,6 +203,16 @@ func.func @test_expression(%arg0: i32, %arg1: i32, %arg2: i32, %arg3: f32, %arg4
return %r : i32
}
+func.func @test_expression_multiple_uses(%arg0: i32, %arg1: i32) -> i32 {
+ %r = emitc.expression %arg0, %arg1 : (i32, i32) -> i32 {
+ %a = emitc.rem %arg0, %arg1 : (i32, i32) -> i32
+ %b = emitc.add %a, %arg0 : (i32, i32) -> i32
+ %c = emitc.mul %b, %a : (i32, i32) -> i32
+ emitc.yield %c : i32
+ }
+ return %r : i32
+}
+
func.func @test_for(%arg0 : index, %arg1 : index, %arg2 : index) {
emitc.for %i0 = %arg0 to %arg1 step %arg2 {
%0 = emitc.call_opaque "func_const"(%i0) : (index) -> i32
diff --git a/mlir/test/Target/Cpp/expressions.mlir b/mlir/test/Target/Cpp/expressions.mlir
index 433a67ccb3f39..4281f41d0b3fb 100644
--- a/mlir/test/Target/Cpp/expressions.mlir
+++ b/mlir/test/Target/Cpp/expressions.mlir
@@ -1,6 +1,25 @@
// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s -check-prefix=CPP-DEFAULT
// RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s -check-prefix=CPP-DECLTOP
+// CPP-DEFAULT: bool single_expression(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DEFAULT-NEXT: return [[VAL_1]] * 42 - [[VAL_2]] < [[VAL_3]];
+// CPP-DEFAULT-NEXT: }
+
+// CPP-DECLTOP: bool single_expression(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DECLTOP-NEXT: return [[VAL_1]] * 42 - [[VAL_2]] < [[VAL_3]];
+// CPP-DECLTOP-NEXT: }
+
+func.func @single_expression(%arg0: i32, %arg1: i32, %arg2: i32) -> i1 {
+ %e = emitc.expression %arg0, %arg1, %arg2 : (i32, i32, i32) -> i1 {
+ %c42 = "emitc.constant"(){value = 42 : i32} : () -> i32
+ %a = emitc.mul %arg0, %c42 : (i32, i32) -> i32
+ %b = emitc.sub %a, %arg1 : (i32, i32) -> i32
+ %c = emitc.cmp lt, %b, %arg2 :(i32, i32) -> i1
+ emitc.yield %c : i1
+ }
+ return %e : i1
+}
+
// CPP-DEFAULT: int32_t single_use(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]], int32_t [[VAL_4:v[0-9]+]]) {
// CPP-DEFAULT-NEXT: bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * M_PI, [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
// CPP-DEFAULT-NEXT: int32_t [[VAL_6:v[0-9]+]];
@@ -185,7 +204,7 @@ func.func @user_with_expression_trait(%arg0: i32, %arg1: i32, %arg2: i32) -> i32
}
// CPP-DEFAULT: int32_t multiple_uses(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]], int32_t [[VAL_4:v[0-9]+]]) {
-// CPP-DEFAULT-NEXT: bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
+// CPP-DEFAULT-NEXT: bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_1]] * [[VAL_2]] < [[VAL_4]];
// CPP-DEFAULT-NEXT: int32_t [[VAL_6:v[0-9]+]];
// CPP-DEFAULT-NEXT: if ([[VAL_5]]) {
// CPP-DEFAULT-NEXT: [[VAL_6]] = [[VAL_1]];
@@ -203,7 +222,7 @@ func.func @user_with_expression_trait(%arg0: i32, %arg1: i32, %arg2: i32) -> i32
// CPP-DECLTOP-NEXT: int32_t [[VAL_6:v[0-9]+]];
// CPP-DECLTOP-NEXT: bool [[VAL_7:v[0-9]+]];
// CPP-DECLTOP-NEXT: int32_t [[VAL_8:v[0-9]+]];
-// CPP-DECLTOP-NEXT: [[VAL_5]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
+// CPP-DECLTOP-NEXT: [[VAL_5]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_1]] * [[VAL_2]] < [[VAL_4]];
// CPP-DECLTOP-NEXT: ;
// CPP-DECLTOP-NEXT: if ([[VAL_5]]) {
// CPP-DECLTOP-NEXT: [[VAL_6]] = [[VAL_1]];
@@ -220,8 +239,8 @@ func.func @multiple_uses(%arg0: i32, %arg1: i32, %arg2: i32, %arg3: i32) -> i32
%e = emitc.expression %arg0, %arg1, %arg2, %arg3 : (i32, i32, i32, i32) -> i1 {
%a = emitc.mul %arg0, %arg1 : (i32, i32) -> i32
%b = emitc.call_opaque "bar" (%a, %arg2) : (i32, i32) -> (i32)
- %c = emitc.sub %b, %arg3 : (i32, i32) -> i32
- %d = emitc.cmp lt, %c, %arg1 :(i32, i32) -> i1
+ %c = emitc.sub %b, %a : (i32, i32) -> i32
+ %d = emitc.cmp lt, %c, %arg3 :(i32, i32) -> i1
emitc.yield %d : i1
}
%v = "emitc.variable"(){value = #emitc.opaque<"">} : () -> !emitc.lvalue<i32>
|
|
@llvm/pr-subscribers-mlir-emitc Author: Gil Rapaport (aniragil) ChangesThe
The first issue was recently resolved by making Full diff: https://github.com/llvm/llvm-project/pull/156450.diff 7 Files Affected:
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index fb7a108b39fc0..721f9f6b320ad 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -402,7 +402,8 @@ def EmitC_CmpOp : EmitC_BinaryOp<"cmp", []> {
let assemblyFormat = "$predicate `,` operands attr-dict `:` functional-type(operands, results)";
}
-def EmitC_ConstantOp : EmitC_Op<"constant", [ConstantLike]> {
+def EmitC_ConstantOp
+ : EmitC_Op<"constant", [ConstantLike, CExpressionInterface]> {
let summary = "Constant operation";
let description = [{
The `emitc.constant` operation produces an SSA value equal to some constant
@@ -429,6 +430,13 @@ def EmitC_ConstantOp : EmitC_Op<"constant", [ConstantLike]> {
let hasFolder = 1;
let hasVerifier = 1;
+
+ let extraClassDeclaration = [{
+ bool hasSideEffects() {
+ // If operand is fundamental type, the operation is pure.
+ return !isFundamentalType(getResult().getType());
+ }
+ }];
}
def EmitC_DivOp : EmitC_BinaryOp<"div", []> {
@@ -466,8 +474,9 @@ def EmitC_ExpressionOp
its single-basic-block region. The operation doesn't take any arguments.
As the operation is to be emitted as a C expression, the operations within
- its body must form a single Def-Use tree of emitc ops whose result is
- yielded by a terminating `emitc.yield`.
+ its body must form a single Def-Use tree, or a DAG trivially expandable to
+ one, i.e. a DAG where each operation with side effects is only reachable
+ once from the expression root.
Example:
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 00ce3b59bf870..a73470cdf76c5 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -14,7 +14,9 @@
#include "mlir/IR/DialectImplementation.h"
#include "mlir/IR/Types.h"
#include "mlir/Interfaces/FunctionImplementation.h"
+#include "mlir/Support/LLVM.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/TypeSwitch.h"
#include "llvm/Support/Casting.h"
@@ -462,12 +464,34 @@ LogicalResult ExpressionOp::verify() {
return emitOpError("requires yielded type to match return type");
for (Operation &op : region.front().without_terminator()) {
- if (!isa<emitc::CExpressionInterface>(op))
+ auto expressionInterface = dyn_cast<emitc::CExpressionInterface>(op);
+ if (!expressionInterface)
return emitOpError("contains an unsupported operation");
if (op.getNumResults() != 1)
return emitOpError("requires exactly one result for each operation");
- if (!op.getResult(0).hasOneUse())
- return emitOpError("requires exactly one use for each operation");
+ Value result = op.getResult(0);
+ if (result.use_empty())
+ return emitOpError("contains an unused operation");
+ }
+
+ // Make sure any operation with side effect is only reachable once from
+ // the root op, otherwise emission will be replicating side effects.
+ SmallPtrSet<Operation *, 16> visited;
+ SmallVector<Operation *> worklist;
+ worklist.push_back(rootOp);
+ while (!worklist.empty()) {
+ Operation *op = worklist.back();
+ worklist.pop_back();
+ if (visited.contains(op)) {
+ if (cast<CExpressionInterface>(op).hasSideEffects())
+ return emitOpError(
+ "requires exactly one use for operations with side effects");
+ }
+ visited.insert(op);
+ for (Value operand : op->getOperands())
+ if (Operation *def = operand.getDefiningOp()) {
+ worklist.push_back(def);
+ }
}
return success();
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 570f38c60020b..a5bd80e9d6b8b 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -97,6 +97,7 @@ static FailureOr<int> getOperatorPrecedence(Operation *operation) {
return op->emitError("unsupported cmp predicate");
})
.Case<emitc::ConditionalOp>([&](auto op) { return 2; })
+ .Case<emitc::ConstantOp>([&](auto op) { return 17; })
.Case<emitc::DivOp>([&](auto op) { return 13; })
.Case<emitc::LoadOp>([&](auto op) { return 16; })
.Case<emitc::LogicalAndOp>([&](auto op) { return 4; })
@@ -267,8 +268,14 @@ struct CppEmitter {
Operation *def = value.getDefiningOp();
if (!def)
return false;
+ return isPartOfCurrentExpression(def);
+ }
+
+ /// Determine whether given operation is part of the expression potentially
+ /// being emitted.
+ bool isPartOfCurrentExpression(Operation *def) {
auto operandExpression = dyn_cast<ExpressionOp>(def->getParentOp());
- return operandExpression == emittedExpression;
+ return operandExpression && operandExpression == emittedExpression;
};
// Resets the value counter to 0.
@@ -408,6 +415,9 @@ static LogicalResult printOperation(CppEmitter &emitter,
Operation *operation = constantOp.getOperation();
Attribute value = constantOp.getValue();
+ if (emitter.isPartOfCurrentExpression(operation))
+ return emitter.emitAttribute(operation->getLoc(), value);
+
return printConstantOp(emitter, operation, value);
}
diff --git a/mlir/test/Dialect/EmitC/form-expressions.mlir b/mlir/test/Dialect/EmitC/form-expressions.mlir
index 67cd6fddba638..7b6723989e260 100644
--- a/mlir/test/Dialect/EmitC/form-expressions.mlir
+++ b/mlir/test/Dialect/EmitC/form-expressions.mlir
@@ -2,8 +2,8 @@
// CHECK-LABEL: func.func @single_expression(
// CHECK-SAME: %[[VAL_0:.*]]: i32, %[[VAL_1:.*]]: i32, %[[VAL_2:.*]]: i32, %[[VAL_3:.*]]: i32) -> i1 {
-// CHECK: %[[VAL_4:.*]] = "emitc.constant"() <{value = 42 : i32}> : () -> i32
-// CHECK: %[[VAL_5:.*]] = emitc.expression %[[VAL_3]], %[[VAL_2]], %[[VAL_0]], %[[VAL_4]] : (i32, i32, i32, i32) -> i1 {
+// CHECK: %[[VAL_5:.*]] = emitc.expression %[[VAL_3]], %[[VAL_2]], %[[VAL_0]] : (i32, i32, i32) -> i1 {
+// CHECK: %[[VAL_4:.*]] = "emitc.constant"() <{value = 42 : i32}> : () -> i32
// CHECK: %[[VAL_6:.*]] = mul %[[VAL_0]], %[[VAL_4]] : (i32, i32) -> i32
// CHECK: %[[VAL_7:.*]] = sub %[[VAL_6]], %[[VAL_2]] : (i32, i32) -> i32
// CHECK: %[[VAL_8:.*]] = cmp lt, %[[VAL_7]], %[[VAL_3]] : (i32, i32) -> i1
@@ -131,7 +131,10 @@ func.func @single_result_requirement() -> (i32, i32) {
// CHECK-LABEL: func.func @expression_with_load(
// CHECK-SAME: %[[VAL_0:.*]]: i32,
// CHECK-SAME: %[[VAL_1:.*]]: !emitc.ptr<i32>) -> i1 {
-// CHECK: %[[VAL_2:.*]] = "emitc.constant"() <{value = 0 : i64}> : () -> i64
+// CHECK: %[[VAL_2:.*]] = emitc.expression : () -> i64 {
+// CHECK: %[[VAL_C:.*]] = "emitc.constant"() <{value = 0 : i64}> : () -> i64
+// CHECK: yield %[[VAL_C]] : i64
+// CHECK: }
// CHECK: %[[VAL_3:.*]] = "emitc.variable"() <{value = #emitc.opaque<"42">}> : () -> !emitc.lvalue<i32>
// CHECK: %[[VAL_4:.*]] = emitc.expression %[[VAL_3]] : (!emitc.lvalue<i32>) -> i32 {
// CHECK: %[[VAL_5:.*]] = load %[[VAL_3]] : <i32>
@@ -162,24 +165,46 @@ func.func @expression_with_load(%arg0: i32, %arg1: !emitc.ptr<i32>) -> i1 {
}
// CHECK-LABEL: func.func @opaque_type_expression(%arg0: i32, %arg1: !emitc.opaque<"T0">, %arg2: i32) -> i1 {
-// CHECK: %0 = "emitc.constant"() <{value = 42 : i32}> : () -> i32
-// CHECK: %1 = emitc.expression %arg1, %arg0, %0 : (!emitc.opaque<"T0">, i32, i32) -> i32 {
-// CHECK: %3 = mul %arg0, %0 : (i32, i32) -> i32
-// CHECK: %4 = sub %3, %arg1 : (i32, !emitc.opaque<"T0">) -> i32
+// CHECK: %0 = emitc.expression : () -> !emitc.opaque<"T1"> {
+// CHECK: %4 = "emitc.constant"() <{value = #emitc.opaque<"V">}> : () -> !emitc.opaque<"T1">
+// CHECK: yield %4 : !emitc.opaque<"T1">
+// CHECK: }
+// CHECK: %1 = emitc.expression %arg0, %0 : (i32, !emitc.opaque<"T1">) -> i32 {
+// CHECK: %4 = mul %arg0, %0 : (i32, !emitc.opaque<"T1">) -> i32
+// CHECK: yield %4 : i32
+// CHECK: }
+// CHECK: %2 = emitc.expression %1, %arg1 : (i32, !emitc.opaque<"T0">) -> i32 {
+// CHECK: %4 = sub %1, %arg1 : (i32, !emitc.opaque<"T0">) -> i32
// CHECK: yield %4 : i32
// CHECK: }
-// CHECK: %2 = emitc.expression %1, %arg2 : (i32, i32) -> i1 {
-// CHECK: %3 = cmp lt, %1, %arg2 : (i32, i32) -> i1
-// CHECK: yield %3 : i1
+// CHECK: %3 = emitc.expression %2, %arg2 : (i32, i32) -> i1 {
+// CHECK: %4 = cmp lt, %2, %arg2 : (i32, i32) -> i1
+// CHECK: yield %4 : i1
// CHECK: }
-// CHECK: return %2 : i1
+// CHECK: return %3 : i1
// CHECK: }
func.func @opaque_type_expression(%arg0: i32, %arg1: !emitc.opaque<"T0">, %arg2: i32) -> i1 {
- %c42 = "emitc.constant"(){value = 42 : i32} : () -> i32
- %a = emitc.mul %arg0, %c42 : (i32, i32) -> i32
+ %c42 = "emitc.constant"(){value = #emitc.opaque<"V">} : () -> !emitc.opaque<"T1">
+ %a = emitc.mul %arg0, %c42 : (i32, !emitc.opaque<"T1">) -> i32
%b = emitc.sub %a, %arg1 : (i32, !emitc.opaque<"T0">) -> i32
%c = emitc.cmp lt, %b, %arg2 :(i32, i32) -> i1
return %c : i1
}
+
+// CHECK-LABEL: func.func @expression_with_constant(
+// CHECK-SAME: %[[VAL_0:.*]]: i32) -> i32 {
+// CHECK: %[[VAL_1:.*]] = emitc.expression %[[VAL_0]] : (i32) -> i32 {
+// CHECK: %[[VAL_2:.*]] = "emitc.constant"() <{value = 42 : i32}> : () -> i32
+// CHECK: %[[VAL_3:.*]] = mul %[[VAL_0]], %[[VAL_2]] : (i32, i32) -> i32
+// CHECK: yield %[[VAL_3]] : i32
+// CHECK: }
+// CHECK: return %[[VAL_1]] : i32
+// CHECK: }
+
+func.func @expression_with_constant(%arg0: i32) -> i32 {
+ %c42 = "emitc.constant"(){value = 42 : i32} : () -> i32
+ %a = emitc.mul %arg0, %c42 : (i32, i32) -> i32
+ return %a : i32
+}
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index fdfb0eb46f7c5..a97474401645c 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -311,7 +311,7 @@ func.func @test_expression_illegal_op(%arg0 : i1) -> i32 {
// -----
func.func @test_expression_no_use(%arg0: i32, %arg1: i32) -> i32 {
- // expected-error @+1 {{'emitc.expression' op requires exactly one use for each operation}}
+ // expected-error @+1 {{'emitc.expression' op contains an unused operation}}
%r = emitc.expression %arg0, %arg1 : (i32, i32) -> i32 {
%a = emitc.add %arg0, %arg1 : (i32, i32) -> i32
%b = emitc.rem %arg0, %arg1 : (i32, i32) -> i32
@@ -323,12 +323,12 @@ func.func @test_expression_no_use(%arg0: i32, %arg1: i32) -> i32 {
// -----
func.func @test_expression_multiple_uses(%arg0: i32, %arg1: i32) -> i32 {
- // expected-error @+1 {{'emitc.expression' op requires exactly one use for each operation}}
+ // expected-error @+1 {{'emitc.expression' op requires exactly one use for operations with side effects}}
%r = emitc.expression %arg0, %arg1 : (i32, i32) -> i32 {
- %a = emitc.rem %arg0, %arg1 : (i32, i32) -> i32
+ %a = emitc.call_opaque "foo"(%arg0, %arg1) : (i32, i32) -> i32
%b = emitc.add %a, %arg0 : (i32, i32) -> i32
- %c = emitc.mul %arg1, %a : (i32, i32) -> i32
- emitc.yield %a : i32
+ %c = emitc.mul %b, %a : (i32, i32) -> i32
+ emitc.yield %c : i32
}
return %r : i32
}
diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir
index e890f77173de7..84c9b65d775d2 100644
--- a/mlir/test/Dialect/EmitC/ops.mlir
+++ b/mlir/test/Dialect/EmitC/ops.mlir
@@ -203,6 +203,16 @@ func.func @test_expression(%arg0: i32, %arg1: i32, %arg2: i32, %arg3: f32, %arg4
return %r : i32
}
+func.func @test_expression_multiple_uses(%arg0: i32, %arg1: i32) -> i32 {
+ %r = emitc.expression %arg0, %arg1 : (i32, i32) -> i32 {
+ %a = emitc.rem %arg0, %arg1 : (i32, i32) -> i32
+ %b = emitc.add %a, %arg0 : (i32, i32) -> i32
+ %c = emitc.mul %b, %a : (i32, i32) -> i32
+ emitc.yield %c : i32
+ }
+ return %r : i32
+}
+
func.func @test_for(%arg0 : index, %arg1 : index, %arg2 : index) {
emitc.for %i0 = %arg0 to %arg1 step %arg2 {
%0 = emitc.call_opaque "func_const"(%i0) : (index) -> i32
diff --git a/mlir/test/Target/Cpp/expressions.mlir b/mlir/test/Target/Cpp/expressions.mlir
index 433a67ccb3f39..4281f41d0b3fb 100644
--- a/mlir/test/Target/Cpp/expressions.mlir
+++ b/mlir/test/Target/Cpp/expressions.mlir
@@ -1,6 +1,25 @@
// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s -check-prefix=CPP-DEFAULT
// RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s -check-prefix=CPP-DECLTOP
+// CPP-DEFAULT: bool single_expression(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DEFAULT-NEXT: return [[VAL_1]] * 42 - [[VAL_2]] < [[VAL_3]];
+// CPP-DEFAULT-NEXT: }
+
+// CPP-DECLTOP: bool single_expression(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]]) {
+// CPP-DECLTOP-NEXT: return [[VAL_1]] * 42 - [[VAL_2]] < [[VAL_3]];
+// CPP-DECLTOP-NEXT: }
+
+func.func @single_expression(%arg0: i32, %arg1: i32, %arg2: i32) -> i1 {
+ %e = emitc.expression %arg0, %arg1, %arg2 : (i32, i32, i32) -> i1 {
+ %c42 = "emitc.constant"(){value = 42 : i32} : () -> i32
+ %a = emitc.mul %arg0, %c42 : (i32, i32) -> i32
+ %b = emitc.sub %a, %arg1 : (i32, i32) -> i32
+ %c = emitc.cmp lt, %b, %arg2 :(i32, i32) -> i1
+ emitc.yield %c : i1
+ }
+ return %e : i1
+}
+
// CPP-DEFAULT: int32_t single_use(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]], int32_t [[VAL_4:v[0-9]+]]) {
// CPP-DEFAULT-NEXT: bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * M_PI, [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
// CPP-DEFAULT-NEXT: int32_t [[VAL_6:v[0-9]+]];
@@ -185,7 +204,7 @@ func.func @user_with_expression_trait(%arg0: i32, %arg1: i32, %arg2: i32) -> i32
}
// CPP-DEFAULT: int32_t multiple_uses(int32_t [[VAL_1:v[0-9]+]], int32_t [[VAL_2:v[0-9]+]], int32_t [[VAL_3:v[0-9]+]], int32_t [[VAL_4:v[0-9]+]]) {
-// CPP-DEFAULT-NEXT: bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
+// CPP-DEFAULT-NEXT: bool [[VAL_5:v[0-9]+]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_1]] * [[VAL_2]] < [[VAL_4]];
// CPP-DEFAULT-NEXT: int32_t [[VAL_6:v[0-9]+]];
// CPP-DEFAULT-NEXT: if ([[VAL_5]]) {
// CPP-DEFAULT-NEXT: [[VAL_6]] = [[VAL_1]];
@@ -203,7 +222,7 @@ func.func @user_with_expression_trait(%arg0: i32, %arg1: i32, %arg2: i32) -> i32
// CPP-DECLTOP-NEXT: int32_t [[VAL_6:v[0-9]+]];
// CPP-DECLTOP-NEXT: bool [[VAL_7:v[0-9]+]];
// CPP-DECLTOP-NEXT: int32_t [[VAL_8:v[0-9]+]];
-// CPP-DECLTOP-NEXT: [[VAL_5]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_4]] < [[VAL_2]];
+// CPP-DECLTOP-NEXT: [[VAL_5]] = bar([[VAL_1]] * [[VAL_2]], [[VAL_3]]) - [[VAL_1]] * [[VAL_2]] < [[VAL_4]];
// CPP-DECLTOP-NEXT: ;
// CPP-DECLTOP-NEXT: if ([[VAL_5]]) {
// CPP-DECLTOP-NEXT: [[VAL_6]] = [[VAL_1]];
@@ -220,8 +239,8 @@ func.func @multiple_uses(%arg0: i32, %arg1: i32, %arg2: i32, %arg3: i32) -> i32
%e = emitc.expression %arg0, %arg1, %arg2, %arg3 : (i32, i32, i32, i32) -> i1 {
%a = emitc.mul %arg0, %arg1 : (i32, i32) -> i32
%b = emitc.call_opaque "bar" (%a, %arg2) : (i32, i32) -> (i32)
- %c = emitc.sub %b, %arg3 : (i32, i32) -> i32
- %d = emitc.cmp lt, %c, %arg1 :(i32, i32) -> i1
+ %c = emitc.sub %b, %a : (i32, i32) -> i32
+ %d = emitc.cmp lt, %c, %arg3 :(i32, i32) -> i1
emitc.yield %d : i1
}
%v = "emitc.variable"(){value = #emitc.opaque<"">} : () -> !emitc.lvalue<i32>
|
I am fine with the documentation adjustment you made but cannot fully parse the above right now 😉 |
No problem :) |
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
emitc.constantop was so far left out ofemit.expression's as its ConstantLike trait could cause CSE to invalidateemitc.expressionops in two ways:The first issue was recently resolved by making
emitc.expressionisolated-from-above. The second is resolved here by limiting the single-use requirement to CExpressions with side effects, as ops with no side effects can safely be cloned as needed.