[mlir][EmitC] Disallow string attributes as initial values #75310

simon-camp · 2023-12-13T10:07:46Z

No description provided.

llvmbot · 2023-12-13T10:08:17Z

@llvm/pr-subscribers-mlir

Author: Simon Camphausen (simon-camp)

Changes

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

3 Files Affected:

(modified) mlir/lib/Dialect/EmitC/IR/EmitC.cpp (+29-24)
(modified) mlir/test/Dialect/EmitC/invalid_ops.mlir (+8-16)
(modified) mlir/test/Target/Cpp/const.mlir (+3-3)

diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index e8ea4da0b089c..e9379d5b711d9 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 49efb962dfa25..0814e1d42de82 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 e6c94732e9f6b..28a547909a0ac 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;

llvmbot · 2023-12-13T10:08:17Z

@llvm/pr-subscribers-mlir-emitc

Author: Simon Camphausen (simon-camp)

Changes

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

3 Files Affected:

(modified) mlir/lib/Dialect/EmitC/IR/EmitC.cpp (+29-24)
(modified) mlir/test/Dialect/EmitC/invalid_ops.mlir (+8-16)
(modified) mlir/test/Target/Cpp/const.mlir (+3-3)

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;

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

github-actions · 2023-12-19T11:22:25Z

✅ With the latest revision this PR passed the C/C++ code formatter.

aniragil

LGTM!

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

marbre

LGTM, thanks!

llvmbot added mlir mlir:emitc labels Dec 13, 2023

simon-camp requested review from marbre, jpienaar and aniragil December 15, 2023 09:42

aniragil reviewed Dec 17, 2023

View reviewed changes

mlir/lib/Dialect/EmitC/IR/EmitC.cpp Outdated Show resolved Hide resolved

mlir/lib/Dialect/EmitC/IR/EmitC.cpp Outdated Show resolved Hide resolved

mlir/lib/Dialect/EmitC/IR/EmitC.cpp Outdated Show resolved Hide resolved

mlir/lib/Dialect/EmitC/IR/EmitC.cpp Show resolved Hide resolved

simon-camp requested a review from aniragil December 19, 2023 11:43

aniragil approved these changes Dec 19, 2023

View reviewed changes

marbre reviewed Dec 19, 2023

View reviewed changes

mlir/lib/Dialect/EmitC/IR/EmitC.cpp Outdated Show resolved Hide resolved

simon-camp force-pushed the emitc.string-attr branch 2 times, most recently from 5d5fb11 to d68a930 Compare December 20, 2023 14:01

simon-camp mentioned this pull request Dec 20, 2023

[mlir][EmitC] Use declarative assembly format for opaque types and attributes #76066

Merged

simon-camp force-pushed the emitc.string-attr branch from d68a930 to bab0634 Compare January 2, 2024 14:04

marbre self-requested a review January 2, 2024 14:16

marbre approved these changes Jan 2, 2024

View reviewed changes

[mlir][EmitC] Disallow string attributes as initial values

673d3fb

simon-camp force-pushed the emitc.string-attr branch from bab0634 to 673d3fb Compare January 2, 2024 15:32

simon-camp merged commit 795c989 into llvm:main Jan 2, 2024

simon-camp deleted the emitc.string-attr branch January 3, 2024 10:01

mgehre-amd pushed a commit to Xilinx/llvm-project that referenced this pull request Mar 11, 2024

[mlir][EmitC] Disallow string attributes as initial values (llvm#75310)

864aa1d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][EmitC] Disallow string attributes as initial values #75310

[mlir][EmitC] Disallow string attributes as initial values #75310

Uh oh!

simon-camp commented Dec 13, 2023 •

edited

Loading

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 19, 2023 •

edited

Loading

Uh oh!

aniragil left a comment

Uh oh!

Uh oh!

marbre left a comment

Uh oh!

Uh oh!

[mlir][EmitC] Disallow string attributes as initial values #75310

[mlir][EmitC] Disallow string attributes as initial values #75310

Uh oh!

Conversation

simon-camp commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aniragil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marbre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simon-camp commented Dec 13, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading