[Mlir-commits] [mlir] [mlir][EmitC] Use declarative assembly format for opaque types and attributes (PR #76066)
Simon Camphausen
llvmlistbot at llvm.org
Wed Dec 20 07:45:20 PST 2023
https://github.com/simon-camp created https://github.com/llvm/llvm-project/pull/76066
Stacked on top of #75310
>From fc27d9a8d092e0981e6824b50465b2eedc3259b1 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/5] [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 fd32efe783bcf5..047c11ff010f22 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();
}
@@ -561,17 +574,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 6ad646d7c62f11..a02002de30dfb5 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 9618a78ad9b8646764dc8f79f296bf9dc0f39c87 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/5] 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 047c11ff010f22..d0fa187dd1fd64 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 32471a189dfab730fcc136a1c399ab8a47f817f3 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/5] 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 d0fa187dd1fd64..dd2462fe4509c4 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())
@@ -575,8 +576,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 a02002de30dfb5..3caef6078c6ade 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
}
>From d68a930e8690eb4036fe362408885728cdeb7d8a Mon Sep 17 00:00:00 2001
From: Simon Camphausen <simon.camphausen at iml.fraunhofer.de>
Date: Wed, 20 Dec 2023 13:55:26 +0000
Subject: [PATCH 4/5] Refine error message
---
mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 7 ++++---
mlir/test/Dialect/EmitC/invalid_ops.mlir | 24 ++++++++++++++++++++----
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index dd2462fe4509c4..eba1625f72275c 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -64,10 +64,11 @@ static LogicalResult verifyInitializationAttribute(Operation *op,
<< "string attributes are not supported, use #emitc.opaque instead";
Type resultType = op->getResult(0).getType();
Type attrType = cast<TypedAttr>(value).getType();
- if (!llvm::isa<NoneType>(attrType) && resultType != attrType)
+ if (resultType != attrType)
return op->emitOpError()
- << "requires attribute's type (" << attrType
- << ") to match op's result type (" << resultType << ")";
+ << "requires attribute to either be an #emitc.opaque attribute or "
+ "it's type ("
+ << attrType << ") to match op's result type (" << resultType << ")";
return success();
}
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index 3caef6078c6ade..04411d85fc49db 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -8,14 +8,22 @@ 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 result type ('i32')}}
+func.func @const_attribute_return_type_1() {
+ // expected-error @+1 {{'emitc.constant' op requires attribute to either be an #emitc.opaque attribute or it's type ('i64') to match op's result type ('i32')}}
%c0 = "emitc.constant"(){value = 42: i64} : () -> i32
return
}
// -----
+func.func @const_attribute_return_type_2() {
+ // expected-error @+1 {{'emitc.constant' op attribute 'value' failed to satisfy constraint: An opaque attribute or TypedAttr instance}}
+ %c0 = "emitc.constant"(){value = unit} : () -> i32
+ return
+}
+
+// -----
+
func.func @empty_constant() {
// expected-error @+1 {{'emitc.constant' op value must not be empty}}
%c0 = "emitc.constant"(){value = #emitc.opaque<"">} : () -> i32
@@ -97,14 +105,22 @@ 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 result type ('i32')}}
+func.func @var_attribute_return_type_1() {
+ // expected-error @+1 {{'emitc.variable' op requires attribute to either be an #emitc.opaque attribute or it's type ('i64') to match op's result type ('i32')}}
%c0 = "emitc.variable"(){value = 42: i64} : () -> i32
return
}
// -----
+func.func @var_attribute_return_type_2() {
+ // expected-error @+1 {{'emitc.variable' op attribute 'value' failed to satisfy constraint: An opaque attribute or TypedAttr instance}}
+ %c0 = "emitc.variable"(){value = unit} : () -> 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>
>From 4ca79ffe598acf6b02c22d5562cc313617278da6 Mon Sep 17 00:00:00 2001
From: Simon Camphausen <simon.camphausen at iml.fraunhofer.de>
Date: Wed, 20 Dec 2023 15:37:19 +0000
Subject: [PATCH 5/5] [mlir][EmitC] Use declarative assembly format for opaque
types and attributes
---
.../mlir/Dialect/EmitC/IR/EmitCAttributes.td | 3 +-
.../mlir/Dialect/EmitC/IR/EmitCTypes.td | 3 +-
mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 49 +++----------------
3 files changed, 11 insertions(+), 44 deletions(-)
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitCAttributes.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitCAttributes.td
index ae843e49c6c5ba..ea5e9efd5fa0b9 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitCAttributes.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitCAttributes.td
@@ -57,8 +57,7 @@ def EmitC_OpaqueAttr : EmitC_Attr<"Opaque", "opaque"> {
}];
let parameters = (ins StringRefParameter<"the opaque value">:$value);
-
- let hasCustomAssemblyFormat = 1;
+ let assemblyFormat = "`<` $value `>`";
}
def EmitC_OpaqueOrTypedAttr : AnyAttrOf<[EmitC_OpaqueAttr, TypedAttrInterface]>;
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td
index 7874aa2c9e3040..8818c049ed7713 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitCTypes.td
@@ -42,7 +42,8 @@ def EmitC_OpaqueType : EmitC_Type<"Opaque", "opaque"> {
}];
let parameters = (ins StringRefParameter<"the opaque value">:$value);
- let hasCustomAssemblyFormat = 1;
+ let assemblyFormat = "`<` $value `>`";
+ let genVerifyDecl = 1;
}
def EmitC_PointerType : EmitC_Type<"Pointer", "ptr"> {
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index eba1625f72275c..f78e605791186e 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -617,27 +617,6 @@ LogicalResult emitc::YieldOp::verify() {
#define GET_ATTRDEF_CLASSES
#include "mlir/Dialect/EmitC/IR/EmitCAttributes.cpp.inc"
-Attribute emitc::OpaqueAttr::parse(AsmParser &parser, Type type) {
- if (parser.parseLess())
- return Attribute();
- std::string value;
- SMLoc loc = parser.getCurrentLocation();
- if (parser.parseOptionalString(&value)) {
- parser.emitError(loc) << "expected string";
- return Attribute();
- }
- if (parser.parseGreater())
- return Attribute();
-
- return get(parser.getContext(), value);
-}
-
-void emitc::OpaqueAttr::print(AsmPrinter &printer) const {
- printer << "<\"";
- llvm::printEscapedString(getValue(), printer.getStream());
- printer << "\">";
-}
-
//===----------------------------------------------------------------------===//
// EmitC Types
//===----------------------------------------------------------------------===//
@@ -649,27 +628,15 @@ void emitc::OpaqueAttr::print(AsmPrinter &printer) const {
// OpaqueType
//===----------------------------------------------------------------------===//
-Type emitc::OpaqueType::parse(AsmParser &parser) {
- if (parser.parseLess())
- return Type();
- std::string value;
- SMLoc loc = parser.getCurrentLocation();
- if (parser.parseOptionalString(&value) || value.empty()) {
- parser.emitError(loc) << "expected non empty string in !emitc.opaque type";
- return Type();
+LogicalResult mlir::emitc::OpaqueType::verify(
+ llvm::function_ref<mlir::InFlightDiagnostic()> emitError,
+ llvm::StringRef value) {
+ if (value.empty()) {
+ return emitError() << "expected non empty string in !emitc.opaque type";
}
if (value.back() == '*') {
- parser.emitError(loc) << "pointer not allowed as outer type with "
- "!emitc.opaque, use !emitc.ptr instead";
- return Type();
+ return emitError() << "pointer not allowed as outer type with "
+ "!emitc.opaque, use !emitc.ptr instead";
}
- if (parser.parseGreater())
- return Type();
- return get(parser.getContext(), value);
-}
-
-void emitc::OpaqueType::print(AsmPrinter &printer) const {
- printer << "<\"";
- llvm::printEscapedString(getValue(), printer.getStream());
- printer << "\">";
+ return success();
}
More information about the Mlir-commits
mailing list