[Mlir-commits] [mlir] [mlir][EmitC] Disallow string attributes as initial values (PR #75310)
Simon Camphausen
llvmlistbot at llvm.org
Tue Dec 19 03:40:45 PST 2023
https://github.com/simon-camp updated https://github.com/llvm/llvm-project/pull/75310
>From bec4f80bd86e05737405361e2267b3f04c3f5f57 Mon Sep 17 00:00:00 2001
From: Simon Camphausen <simon.camphausen at iml.fraunhofer.de>
Date: Tue, 12 Dec 2023 15:44:27 +0000
Subject: [PATCH 1/3] [mlir][EmitC] Disallow string attributes as initial
values
---
mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 53 +++++++++++++-----------
mlir/test/Dialect/EmitC/invalid_ops.mlir | 24 ++++-------
mlir/test/Target/Cpp/const.mlir | 6 +--
3 files changed, 40 insertions(+), 43 deletions(-)
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index e8ea4da0b089c8..e9379d5b711d9e 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -50,6 +50,26 @@ void mlir::emitc::buildTerminatedBody(OpBuilder &builder, Location loc) {
builder.create<emitc::YieldOp>(loc);
}
+/// Check that the type of the initial value is compatible with the operations
+/// result type.
+LogicalResult verifyInitializationAttribute(Operation *op, Attribute value,
+ Type expectedType) {
+ if (llvm::isa<emitc::OpaqueAttr>(value))
+ return success();
+
+ if (llvm::isa<StringAttr>(value))
+ return op->emitOpError()
+ << "string attributes are not supported, use #emitc.opaque instead";
+
+ auto typedValue = cast<TypedAttr>(value);
+ if (!llvm::isa<NoneType>(typedValue.getType()) &&
+ expectedType != typedValue.getType())
+ return op->emitOpError()
+ << "requires attribute's type (" << typedValue.getType()
+ << ") to match op's return type (" << expectedType << ")";
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// AddOp
//===----------------------------------------------------------------------===//
@@ -169,21 +189,14 @@ LogicalResult emitc::CallOpaqueOp::verify() {
// ConstantOp
//===----------------------------------------------------------------------===//
-/// The constant op requires that the attribute's type matches the return type.
LogicalResult emitc::ConstantOp::verify() {
- if (llvm::isa<emitc::OpaqueAttr>(getValueAttr()))
- return success();
-
- // Value must not be empty
- StringAttr strAttr = llvm::dyn_cast<StringAttr>(getValueAttr());
- if (strAttr && strAttr.empty())
- return emitOpError() << "value must not be empty";
-
- auto value = cast<TypedAttr>(getValueAttr());
- Type type = getType();
- if (!llvm::isa<NoneType>(value.getType()) && type != value.getType())
- return emitOpError() << "requires attribute's type (" << value.getType()
- << ") to match op's return type (" << type << ")";
+ Attribute value = getValueAttr();
+ if (failed(verifyInitializationAttribute(getOperation(), value, getType())))
+ return failure();
+ if (auto opaqueValue = llvm::dyn_cast<emitc::OpaqueAttr>(value)) {
+ if (opaqueValue.getValue().empty())
+ return emitOpError() << "value must not be empty";
+ }
return success();
}
@@ -517,17 +530,9 @@ LogicalResult SubOp::verify() {
// VariableOp
//===----------------------------------------------------------------------===//
-/// The variable op requires that the attribute's type matches the return type.
LogicalResult emitc::VariableOp::verify() {
- if (llvm::isa<emitc::OpaqueAttr>(getValueAttr()))
- return success();
-
- auto value = cast<TypedAttr>(getValueAttr());
- Type type = getType();
- if (!llvm::isa<NoneType>(value.getType()) && type != value.getType())
- return emitOpError() << "requires attribute's type (" << value.getType()
- << ") to match op's return type (" << type << ")";
- return success();
+ return verifyInitializationAttribute(getOperation(), getValueAttr(),
+ getType());
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index 49efb962dfa257..0814e1d42de82b 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -1,16 +1,16 @@
// RUN: mlir-opt %s -split-input-file -verify-diagnostics
-func.func @const_attribute_return_type_1() {
- // expected-error @+1 {{'emitc.constant' op requires attribute's type ('i64') to match op's return type ('i32')}}
- %c0 = "emitc.constant"(){value = 42: i64} : () -> i32
+func.func @const_attribute_str() {
+ // expected-error @+1 {{'emitc.constant' op string attributes are not supported, use #emitc.opaque instead}}
+ %c0 = "emitc.constant"(){value = "NULL"} : () -> !emitc.ptr<i32>
return
}
// -----
-func.func @const_attribute_return_type_2() {
- // expected-error @+1 {{'emitc.constant' op requires attribute's type ('!emitc.opaque<"char">') to match op's return type ('!emitc.opaque<"mychar">')}}
- %c0 = "emitc.constant"(){value = "CHAR_MIN" : !emitc.opaque<"char">} : () -> !emitc.opaque<"mychar">
+func.func @const_attribute_return_type() {
+ // expected-error @+1 {{'emitc.constant' op requires attribute's type ('i64') to match op's return type ('i32')}}
+ %c0 = "emitc.constant"(){value = 42: i64} : () -> i32
return
}
@@ -18,7 +18,7 @@ func.func @const_attribute_return_type_2() {
func.func @empty_constant() {
// expected-error @+1 {{'emitc.constant' op value must not be empty}}
- %c0 = "emitc.constant"(){value = ""} : () -> i32
+ %c0 = "emitc.constant"(){value = #emitc.opaque<"">} : () -> i32
return
}
@@ -97,7 +97,7 @@ func.func @illegal_operand() {
// -----
-func.func @var_attribute_return_type_1() {
+func.func @var_attribute_return_type() {
// expected-error @+1 {{'emitc.variable' op requires attribute's type ('i64') to match op's return type ('i32')}}
%c0 = "emitc.variable"(){value = 42: i64} : () -> i32
return
@@ -105,14 +105,6 @@ func.func @var_attribute_return_type_1() {
// -----
-func.func @var_attribute_return_type_2() {
- // expected-error @+1 {{'emitc.variable' op requires attribute's type ('!emitc.ptr<i64>') to match op's return type ('!emitc.ptr<i32>')}}
- %c0 = "emitc.variable"(){value = "nullptr" : !emitc.ptr<i64>} : () -> !emitc.ptr<i32>
- return
-}
-
-// -----
-
func.func @cast_tensor(%arg : tensor<f32>) {
// expected-error @+1 {{'emitc.cast' op operand type 'tensor<f32>' and result type 'tensor<f32>' are cast incompatible}}
%1 = emitc.cast %arg: tensor<f32> to tensor<f32>
diff --git a/mlir/test/Target/Cpp/const.mlir b/mlir/test/Target/Cpp/const.mlir
index e6c94732e9f6bc..28a547909a0ac6 100644
--- a/mlir/test/Target/Cpp/const.mlir
+++ b/mlir/test/Target/Cpp/const.mlir
@@ -2,7 +2,7 @@
// RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s -check-prefix=CPP-DECLTOP
func.func @emitc_constant() {
- %c0 = "emitc.constant"(){value = #emitc.opaque<"">} : () -> i32
+ %c0 = "emitc.constant"(){value = #emitc.opaque<"INT_MAX">} : () -> i32
%c1 = "emitc.constant"(){value = 42 : i32} : () -> i32
%c2 = "emitc.constant"(){value = -1 : i32} : () -> i32
%c3 = "emitc.constant"(){value = -1 : si8} : () -> si8
@@ -11,7 +11,7 @@ func.func @emitc_constant() {
return
}
// CPP-DEFAULT: void emitc_constant() {
-// CPP-DEFAULT-NEXT: int32_t [[V0:[^ ]*]];
+// CPP-DEFAULT-NEXT: int32_t [[V0:[^ ]*]] = INT_MAX;
// CPP-DEFAULT-NEXT: int32_t [[V1:[^ ]*]] = 42;
// CPP-DEFAULT-NEXT: int32_t [[V2:[^ ]*]] = -1;
// CPP-DEFAULT-NEXT: int8_t [[V3:[^ ]*]] = -1;
@@ -25,7 +25,7 @@ func.func @emitc_constant() {
// CPP-DECLTOP-NEXT: int8_t [[V3:[^ ]*]];
// CPP-DECLTOP-NEXT: uint8_t [[V4:[^ ]*]];
// CPP-DECLTOP-NEXT: char [[V5:[^ ]*]];
-// CPP-DECLTOP-NEXT: ;
+// CPP-DECLTOP-NEXT: [[V0]] = INT_MAX;
// CPP-DECLTOP-NEXT: [[V1]] = 42;
// CPP-DECLTOP-NEXT: [[V2]] = -1;
// CPP-DECLTOP-NEXT: [[V3]] = -1;
>From 21aabe981a34d9d75788f7aea9a68af1b13f7e61 Mon Sep 17 00:00:00 2001
From: Simon Camphausen <simon.camphausen at iml.fraunhofer.de>
Date: Tue, 19 Dec 2023 12:19:49 +0100
Subject: [PATCH 2/3] Apply suggestions from code review
Co-authored-by: Gil Rapaport <aniragil at gmail.com>
---
mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index e9379d5b711d9e..7da8e7ef53b0f7 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -52,7 +52,7 @@ void mlir::emitc::buildTerminatedBody(OpBuilder &builder, Location loc) {
/// Check that the type of the initial value is compatible with the operations
/// result type.
-LogicalResult verifyInitializationAttribute(Operation *op, Attribute value,
+static LogicalResult verifyInitializationAttribute(Operation *op, Attribute value,
Type expectedType) {
if (llvm::isa<emitc::OpaqueAttr>(value))
return success();
>From 7f4df33d832b2af7dd6e7a1cb4f272adf73b1660 Mon Sep 17 00:00:00 2001
From: Simon Camphausen <simon.camphausen at iml.fraunhofer.de>
Date: Tue, 19 Dec 2023 11:40:00 +0000
Subject: [PATCH 3/3] Extract result type from operation
Co-authored-by: Gil Rapaport <aniragil at gmail.com>
---
mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 22 +++++++++++-----------
mlir/test/Dialect/EmitC/invalid_ops.mlir | 4 ++--
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 7da8e7ef53b0f7..d27d8a0b7b3946 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -52,21 +52,22 @@ void mlir::emitc::buildTerminatedBody(OpBuilder &builder, Location loc) {
/// Check that the type of the initial value is compatible with the operations
/// result type.
-static LogicalResult verifyInitializationAttribute(Operation *op, Attribute value,
- Type expectedType) {
+static LogicalResult verifyInitializationAttribute(Operation *op,
+ Attribute value) {
+ assert(op->getNumResults() == 1 && "operation must have 1 result");
+
if (llvm::isa<emitc::OpaqueAttr>(value))
return success();
if (llvm::isa<StringAttr>(value))
return op->emitOpError()
<< "string attributes are not supported, use #emitc.opaque instead";
-
- auto typedValue = cast<TypedAttr>(value);
- if (!llvm::isa<NoneType>(typedValue.getType()) &&
- expectedType != typedValue.getType())
+ Type resultType = op->getResult(0).getType();
+ Type attrType = cast<TypedAttr>(value).getType();
+ if (!llvm::isa<NoneType>(attrType) && resultType != attrType)
return op->emitOpError()
- << "requires attribute's type (" << typedValue.getType()
- << ") to match op's return type (" << expectedType << ")";
+ << "requires attribute's type (" << attrType
+ << ") to match op's result type (" << resultType << ")";
return success();
}
@@ -191,7 +192,7 @@ LogicalResult emitc::CallOpaqueOp::verify() {
LogicalResult emitc::ConstantOp::verify() {
Attribute value = getValueAttr();
- if (failed(verifyInitializationAttribute(getOperation(), value, getType())))
+ if (failed(verifyInitializationAttribute(getOperation(), value)))
return failure();
if (auto opaqueValue = llvm::dyn_cast<emitc::OpaqueAttr>(value)) {
if (opaqueValue.getValue().empty())
@@ -531,8 +532,7 @@ LogicalResult SubOp::verify() {
//===----------------------------------------------------------------------===//
LogicalResult emitc::VariableOp::verify() {
- return verifyInitializationAttribute(getOperation(), getValueAttr(),
- getType());
+ return verifyInitializationAttribute(getOperation(), getValueAttr());
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index 0814e1d42de82b..6c1a9039d2c02a 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -9,7 +9,7 @@ func.func @const_attribute_str() {
// -----
func.func @const_attribute_return_type() {
- // expected-error @+1 {{'emitc.constant' op requires attribute's type ('i64') to match op's return type ('i32')}}
+ // expected-error @+1 {{'emitc.constant' op requires attribute's type ('i64') to match op's result type ('i32')}}
%c0 = "emitc.constant"(){value = 42: i64} : () -> i32
return
}
@@ -98,7 +98,7 @@ func.func @illegal_operand() {
// -----
func.func @var_attribute_return_type() {
- // expected-error @+1 {{'emitc.variable' op requires attribute's type ('i64') to match op's return type ('i32')}}
+ // expected-error @+1 {{'emitc.variable' op requires attribute's type ('i64') to match op's result type ('i32')}}
%c0 = "emitc.variable"(){value = 42: i64} : () -> i32
return
}
More information about the Mlir-commits
mailing list