From cbc70ebbd07f71ab65abf0a9e2520871cd255cad Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Thu, 14 Mar 2024 21:26:55 +0100 Subject: [PATCH 1/9] EmitC: Add emitc.global and emitc.get_global This adds - `emitc.global` and `emitc.get_global` ops to model global variables similar to how `memref.global` and `memref.get_global` work. - translation of those ops to C++ - lowering of `memref.global` and `memref.get_global` into those ops --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 66 +++++++++ .../MemRefToEmitC/MemRefToEmitC.cpp | 55 +++++++- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 127 +++++++++++++++++- mlir/lib/Target/Cpp/TranslateToCpp.cpp | 53 +++++++- .../MemRefToEmitC/memref-to-emitc.mlir | 12 ++ mlir/test/Dialect/EmitC/invalid_ops.mlir | 15 +++ mlir/test/Dialect/EmitC/ops.mlir | 12 ++ mlir/test/Target/Cpp/global.mlir | 25 ++++ 8 files changed, 350 insertions(+), 15 deletions(-) create mode 100644 mlir/test/Target/Cpp/global.mlir diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index 75c02bd1d9382..c265601224d58 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -1016,6 +1016,72 @@ def EmitC_VariableOp : EmitC_Op<"variable", []> { let hasVerifier = 1; } +def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { + let summary = "A global variable"; + let description = [{ + The `emitc.global` operation declares or defines a named global variable. + The backing memory for the variable is allocated statically and is + described by the type of the variable. + The operation is a declaration if no `initial_value` is + specified, else it is a definition. The `initial_value` can either be a unit + attribute to represent a definition of an uninitialized global variable, or + an elements attribute to represent the definition of a global variable with + an initial value. The global variable can also be marked constant using the + `constant` unit attribute. Writing to such constant global variables is + undefined. + + The global variable can be accessed by using the `emitc.get_global` to + retrieve the value for the global variable. + + Example: + + ```mlir + // Private variable with an initial value. + emitc.global @x : emitc.array<2xf32> = dense<0.0, 2.0> + ``` + }]; + + let arguments = (ins SymbolNameAttr:$sym_name, + TypeAttr:$type, + OptionalAttr:$initial_value, + UnitAttr:$constant); + + let assemblyFormat = [{ + (`constant` $constant^)? + $sym_name `:` + custom($type, $initial_value) + attr-dict + }]; + + let extraClassDeclaration = [{ + bool isExternal() { return !getInitialValue(); } + bool isUninitialized() { + return !isExternal() && ::llvm::isa(*getInitialValue()); + } + }]; + let hasVerifier = 1; +} + +def EmitC_GetGlobalOp : EmitC_Op<"get_global", + [Pure, DeclareOpInterfaceMethods]> { + let summary = "Obtain access to a global variable"; + let description = [{ + The `emitc.get_global` operation retrieves the lvalue of a + named global variable. If the global variable is marked constant, assigning + to that lvalue is undefined. + + Example: + + ```mlir + %x = emitc.get_global @foo : !emitc.array<2xf32> + ``` + }]; + + let arguments = (ins FlatSymbolRefAttr:$name); + let results = (outs AnyType:$result); + let assemblyFormat = "$name `:` type($result) attr-dict"; +} + def EmitC_VerbatimOp : EmitC_Op<"verbatim"> { let summary = "Verbatim operation"; let description = [{ diff --git a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp index 7eedd3a8769ff..d27d1e21a87c8 100644 --- a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp +++ b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp @@ -108,6 +108,57 @@ struct ConvertAlloca final : public OpConversionPattern { } }; +struct ConvertGlobal final : public OpConversionPattern { + using OpConversionPattern::OpConversionPattern; + + LogicalResult + matchAndRewrite(memref::GlobalOp op, OpAdaptor operands, + ConversionPatternRewriter &rewriter) const override { + + if (!op.getType().hasStaticShape()) { + return rewriter.notifyMatchFailure( + op.getLoc(), "cannot transform global with dynamic shape"); + } + + if (op.getAlignment().value_or(1) > 1) { + // TODO: Allow alignment if it is not more than the natural alignment + // of the C array. + return rewriter.notifyMatchFailure( + op.getLoc(), "cannot global alloca with alignment requirement"); + } + + auto resultTy = getTypeConverter()->convertType(op.getType()); + if (!resultTy) { + return rewriter.notifyMatchFailure(op.getLoc(), + "cannot convert result type"); + } + + rewriter.replaceOpWithNewOp( + op, operands.getSymName(), resultTy, operands.getInitialValueAttr(), + operands.getConstant()); + return success(); + } +}; + +struct ConvertGetGlobal final + : public OpConversionPattern { + using OpConversionPattern::OpConversionPattern; + + LogicalResult + matchAndRewrite(memref::GetGlobalOp op, OpAdaptor operands, + ConversionPatternRewriter &rewriter) const override { + + auto resultTy = getTypeConverter()->convertType(op.getType()); + if (!resultTy) { + return rewriter.notifyMatchFailure(op.getLoc(), + "cannot convert result type"); + } + rewriter.replaceOpWithNewOp(op, resultTy, + operands.getNameAttr()); + return success(); + } +}; + struct ConvertMemRefToEmitCPass : public impl::ConvertMemRefToEmitCBase { void runOnOperation() override { @@ -161,8 +212,8 @@ void mlir::populateMemRefToEmitCConversionPatterns(RewritePatternSet &patterns, converter); populateCallOpTypeConversionPattern(patterns, converter); populateReturnOpTypeConversionPattern(patterns, converter); - patterns.add(converter, - patterns.getContext()); + patterns.add(converter, patterns.getContext()); } std::unique_ptr> mlir::createConvertMemRefToEmitCPass() { diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index d33e398dfc01f..3beb2722e92db 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -790,13 +790,6 @@ LogicalResult emitc::YieldOp::verify() { return success(); } -//===----------------------------------------------------------------------===// -// TableGen'd op method definitions -//===----------------------------------------------------------------------===// - -#define GET_OP_CLASSES -#include "mlir/Dialect/EmitC/IR/EmitC.cpp.inc" - //===----------------------------------------------------------------------===// // EmitC Enums //===----------------------------------------------------------------------===// @@ -896,3 +889,123 @@ LogicalResult mlir::emitc::OpaqueType::verify( } return success(); } + +//===----------------------------------------------------------------------===// +// GlobalOp +//===----------------------------------------------------------------------===// + +static void printEmitCGlobalOpTypeAndInitialValue(OpAsmPrinter &p, GlobalOp op, + TypeAttr type, + Attribute initialValue) { + p << type; + if (!op.isExternal()) { + p << " = "; + if (op.isUninitialized()) + p << "uninitialized"; + else + p.printAttributeWithoutType(initialValue); + } +} + +static Type getInitializerTypeForGlobal(Type type) { + if (auto array = llvm::dyn_cast(type)) + return RankedTensorType::get(array.getShape(), array.getElementType()); + return type; +} + +static ParseResult +parseEmitCGlobalOpTypeAndInitialValue(OpAsmParser &parser, TypeAttr &typeAttr, + Attribute &initialValue) { + Type type; + if (parser.parseType(type)) + return failure(); + + typeAttr = TypeAttr::get(type); + + if (parser.parseOptionalEqual()) + return success(); + + if (succeeded(parser.parseOptionalKeyword("uninitialized"))) { + initialValue = UnitAttr::get(parser.getContext()); + return success(); + } + + if (parser.parseAttribute(initialValue, getInitializerTypeForGlobal(type))) + return failure(); + if (!llvm::isa(initialValue)) + return parser.emitError(parser.getNameLoc()) + << "initial value should be a unit, integer, float or elements " + "attribute"; + return success(); +} + +LogicalResult GlobalOp::verify() { + // Verify that the initial value, if present, is either a unit attribute or + // an elements attribute. + if (getInitialValue().has_value()) { + Attribute initValue = getInitialValue().value(); + // Check that the type of the initial value is compatible with the type of + // the global variable. + if (auto elementsAttr = llvm::dyn_cast(initValue)) { + auto arrayType = llvm::dyn_cast(getType()); + if (!arrayType) + return emitOpError("expected array type, but got ") << getType(); + + Type initType = elementsAttr.getType(); + Type tensorType = getInitializerTypeForGlobal(getType()); + if (initType != tensorType) { + return emitOpError("initial value expected to be of type ") + << getType() << ", but was of type " << initType; + } + } else if (auto intAttr = dyn_cast(initValue)) { + if (intAttr.getType() != getType()) { + return emitOpError("initial value expected to be of type ") + << getType() << ", but was of type " << intAttr.getType(); + } + } else if (auto floatAttr = dyn_cast(initValue)) { + if (floatAttr.getType() != getType()) { + return emitOpError("initial value expected to be of type ") + << getType() << ", but was of type " << floatAttr.getType(); + } + } else if (isa(initValue)) { + if (getConstant()) { + return emitOpError("cannot declare uninitialized constant"); + } + } else { + return emitOpError( + "initial value should be a unit, integer, float or elements " + "attribute, but got ") + << initValue; + } + } + return success(); +} + +//===----------------------------------------------------------------------===// +// GetGlobalOp +//===----------------------------------------------------------------------===// + +LogicalResult +GetGlobalOp::verifySymbolUses(SymbolTableCollection &symbolTable) { + // Verify that the result type is same as the type of the referenced + // memref.global op. + auto global = + symbolTable.lookupNearestSymbolFrom(*this, getNameAttr()); + if (!global) + return emitOpError("'") + << getName() << "' does not reference a valid emitc.global"; + + Type resultType = getResult().getType(); + if (global.getType() != resultType) + return emitOpError("result type ") + << resultType << " does not match type " << global.getType() + << " of the global @" << getName(); + return success(); +} + +//===----------------------------------------------------------------------===// +// TableGen'd op method definitions +//===----------------------------------------------------------------------===// + +#define GET_OP_CLASSES +#include "mlir/Dialect/EmitC/IR/EmitC.cpp.inc" diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index 92acd145d3a30..ff9b52d47ca99 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -152,6 +152,9 @@ struct CppEmitter { /// any result type could not be converted. LogicalResult emitAssignPrefix(Operation &op); + /// Emits a global variable declaration or definition. + LogicalResult emitGlobalVariable(GlobalOp op); + /// Emits a label for the block. LogicalResult emitLabel(Block &block); @@ -342,6 +345,12 @@ static LogicalResult printOperation(CppEmitter &emitter, return printConstantOp(emitter, operation, value); } +static LogicalResult printOperation(CppEmitter &emitter, + emitc::GlobalOp globalOp) { + + return emitter.emitGlobalVariable(globalOp); +} + static LogicalResult printOperation(CppEmitter &emitter, emitc::AssignOp assignOp) { OpResult result = assignOp.getVar().getDefiningOp()->getResult(0); @@ -352,6 +361,13 @@ static LogicalResult printOperation(CppEmitter &emitter, return emitter.emitOperand(assignOp.getValue()); } +static LogicalResult printOperation(CppEmitter &emitter, + emitc::GetGlobalOp op) { + // Add name to cache so that `hasValueInScope` works. + emitter.getOrCreateName(op.getResult()); + return success(); +} + static LogicalResult printOperation(CppEmitter &emitter, emitc::SubscriptOp subscriptOp) { // Add name to cache so that `hasValueInScope` works. @@ -1118,6 +1134,9 @@ StringRef CppEmitter::getOrCreateName(Value val) { if (auto subscript = dyn_cast_if_present(val.getDefiningOp())) { valueMapper.insert(val, getSubscriptName(subscript)); + } else if (auto getGlobal = dyn_cast_if_present( + val.getDefiningOp())) { + valueMapper.insert(val, getGlobal.getName().str()); } else { valueMapper.insert(val, formatv("v{0}", ++valueInScopeCount.top())); } @@ -1383,6 +1402,28 @@ LogicalResult CppEmitter::emitVariableDeclaration(OpResult result, return success(); } +LogicalResult CppEmitter::emitGlobalVariable(GlobalOp op) { + if (op.isExternal()) + os << "extern "; + if (op.getConstant()) + os << "const "; + + if (failed(emitVariableDeclaration(op->getLoc(), op.getType(), + op.getSymName()))) { + return failure(); + } + + std::optional initialValue = op.getInitialValue(); + if (initialValue && !isa(*initialValue)) { + os << " = "; + if (failed(emitAttribute(op->getLoc(), *initialValue))) + return failure(); + } + + os << ";"; + return success(); +} + LogicalResult CppEmitter::emitAssignPrefix(Operation &op) { // If op is being emitted as part of an expression, bail out. if (getEmittedExpression()) @@ -1443,11 +1484,11 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) { emitc::CallOpaqueOp, emitc::CastOp, emitc::CmpOp, emitc::ConditionalOp, emitc::ConstantOp, emitc::DeclareFuncOp, emitc::DivOp, emitc::ExpressionOp, emitc::ForOp, emitc::FuncOp, - emitc::IfOp, emitc::IncludeOp, emitc::LogicalAndOp, - emitc::LogicalNotOp, emitc::LogicalOrOp, emitc::MulOp, - emitc::RemOp, emitc::ReturnOp, emitc::SubOp, emitc::SubscriptOp, - emitc::UnaryMinusOp, emitc::UnaryPlusOp, emitc::VariableOp, - emitc::VerbatimOp>( + emitc::GlobalOp, emitc::GetGlobalOp, emitc::IfOp, + emitc::IncludeOp, emitc::LogicalAndOp, emitc::LogicalNotOp, + emitc::LogicalOrOp, emitc::MulOp, emitc::RemOp, emitc::ReturnOp, + emitc::SubOp, emitc::SubscriptOp, emitc::UnaryMinusOp, + emitc::UnaryPlusOp, emitc::VariableOp, emitc::VerbatimOp>( [&](auto op) { return printOperation(*this, op); }) // Func ops. .Case( @@ -1460,7 +1501,7 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) { if (failed(status)) return failure(); - if (isa(op)) + if (isa(op)) return success(); if (getEmittedExpression() || diff --git a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir index f3bc5a5124bf0..504436183a4fe 100644 --- a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir +++ b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir @@ -45,3 +45,15 @@ func.func @alloca() { %0 = memref.alloca() : memref<4x8xf32> return } +// ----- + +// CHECK-LABEL: globals +module @globals { + memref.global "private" constant @myglobal : memref<3x7xf32> = dense<4.0> + // CHECK emitc.global constant @myglobal : !emitc.array<3x7xf32> = dense<4.000000e+00> + func.func @use_global() { + // CHECK emitc.get_global @myglobal : !emitc.array<3x7xf32> + %0 = memref.get_global @myglobal : memref<3x7xf32> + return + } +} diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index d542ec0679437..0c667bdfe875a 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -387,3 +387,18 @@ func.func @logical_or_resulterror(%arg0: i32, %arg1: i32) { %0 = "emitc.logical_or"(%arg0, %arg1) : (i32, i32) -> i32 return } + +// ----- + +// expected-error @+1 {{'emitc.global' op cannot declare uninitialized constant}} +emitc.global constant @uninit : i32 = uninitialized + +// ----- + +emitc.global @myglobal : !emitc.array<2xf32> + +func.func @use_global(%i: index) { + // expected-error @+1 {{'emitc.get_global' op result type 'f32' does not match type '!emitc.array<2xf32>' of the global @myglobal}} + %0 = emitc.get_global @myglobal : f32 + return +} diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir index 74ac826eace7c..d9e9ddd2f0496 100644 --- a/mlir/test/Dialect/EmitC/ops.mlir +++ b/mlir/test/Dialect/EmitC/ops.mlir @@ -232,3 +232,15 @@ emitc.verbatim "#endif // __cplusplus" emitc.verbatim "typedef int32_t i32;" emitc.verbatim "typedef float f32;" + + +emitc.global @uninit : i32 = uninitialized +emitc.global @myglobal_int : i32 = 4 +emitc.global @myglobal : !emitc.array<2xf32> = dense<4.000000e+00> +emitc.global constant @myconstant : !emitc.array<2xi16> = dense<2> + +func.func @use_global(%i: index) -> f32 { + %0 = emitc.get_global @myglobal : !emitc.array<2xf32> + %1 = emitc.subscript %0[%i] : <2xf32> + return %1 : f32 +} diff --git a/mlir/test/Target/Cpp/global.mlir b/mlir/test/Target/Cpp/global.mlir new file mode 100644 index 0000000000000..f67cac5c9f14e --- /dev/null +++ b/mlir/test/Target/Cpp/global.mlir @@ -0,0 +1,25 @@ +// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s +// RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s + +emitc.global @decl : i8 +// CHECK: extern int8_t decl; + +emitc.global @uninit : i32 = uninitialized +// CHECK: int32_t uninit; + +emitc.global @myglobal_int : i32 = 4 +// CHECK: int32_t myglobal_int = 4; + +emitc.global @myglobal : !emitc.array<2xf32> = dense<4.000000e+00> +// CHECK: float myglobal[2] = {4.000000000e+00f, 4.000000000e+00f}; + +emitc.global constant @myconstant : !emitc.array<2xi16> = dense<2> +// CHECK: const int16_t myconstant[2] = {2, 2}; + +func.func @use_global(%i: index) -> f32 { + %0 = emitc.get_global @myglobal : !emitc.array<2xf32> + %1 = emitc.subscript %0[%i] : <2xf32> + return %1 : f32 + // CHECK: float use_global(size_t v1) { + // CHECK: return myglobal[v1]; +} From 5ff7d033a7ad0372e74ae92f36898b13d8d0101e Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 13:33:21 +0100 Subject: [PATCH 2/9] make `extern` explicit; add alignas comment --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 21 ++++++++++--------- .../MemRefToEmitC/MemRefToEmitC.cpp | 6 +++--- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 20 +++++------------- mlir/test/Dialect/EmitC/invalid_ops.mlir | 4 ++-- mlir/test/Dialect/EmitC/ops.mlir | 4 ++-- mlir/test/Target/Cpp/global.mlir | 9 +++++--- 6 files changed, 29 insertions(+), 35 deletions(-) diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index c265601224d58..d56c58836a2b7 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -1036,28 +1036,29 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { Example: ```mlir - // Private variable with an initial value. + // Global variable with an initial value. emitc.global @x : emitc.array<2xf32> = dense<0.0, 2.0> + // External global variable + emitc.global extern @x : emitc.array<2xf32> + // Constant global variable + emitc.global const @x : i32 = 0 ``` }]; let arguments = (ins SymbolNameAttr:$sym_name, TypeAttr:$type, - OptionalAttr:$initial_value, + OptionalAttr:$initial_value, + UnitAttr:$external, UnitAttr:$constant); let assemblyFormat = [{ - (`constant` $constant^)? - $sym_name `:` - custom($type, $initial_value) - attr-dict + (`extern` $external^)? (`const` $constant^)? + $sym_name `:` custom($type, $initial_value) attr-dict }]; let extraClassDeclaration = [{ - bool isExternal() { return !getInitialValue(); } - bool isUninitialized() { - return !isExternal() && ::llvm::isa(*getInitialValue()); - } + bool isExternal() { return getExternal(); } + bool isConstant() { return getConstant(); } }]; let hasVerifier = 1; } diff --git a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp index d27d1e21a87c8..a9c625f8050cc 100644 --- a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp +++ b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp @@ -121,10 +121,10 @@ struct ConvertGlobal final : public OpConversionPattern { } if (op.getAlignment().value_or(1) > 1) { - // TODO: Allow alignment if it is not more than the natural alignment - // of the C array. + // TODO: Extend GlobalOp to specify alignment via the `alignas` specifier. return rewriter.notifyMatchFailure( - op.getLoc(), "cannot global alloca with alignment requirement"); + op.getLoc(), "global variable with alignment requirement is " + "currently not supported"); } auto resultTy = getTypeConverter()->convertType(op.getType()); diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index 3beb2722e92db..6bb3c42daa301 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -893,17 +893,13 @@ LogicalResult mlir::emitc::OpaqueType::verify( //===----------------------------------------------------------------------===// // GlobalOp //===----------------------------------------------------------------------===// - static void printEmitCGlobalOpTypeAndInitialValue(OpAsmPrinter &p, GlobalOp op, TypeAttr type, Attribute initialValue) { p << type; - if (!op.isExternal()) { + if (initialValue) { p << " = "; - if (op.isUninitialized()) - p << "uninitialized"; - else - p.printAttributeWithoutType(initialValue); + p.printAttributeWithoutType(initialValue); } } @@ -925,13 +921,9 @@ parseEmitCGlobalOpTypeAndInitialValue(OpAsmParser &parser, TypeAttr &typeAttr, if (parser.parseOptionalEqual()) return success(); - if (succeeded(parser.parseOptionalKeyword("uninitialized"))) { - initialValue = UnitAttr::get(parser.getContext()); - return success(); - } - if (parser.parseAttribute(initialValue, getInitializerTypeForGlobal(type))) return failure(); + if (!llvm::isa(initialValue)) return parser.emitError(parser.getNameLoc()) << "initial value should be a unit, integer, float or elements " @@ -967,16 +959,14 @@ LogicalResult GlobalOp::verify() { return emitOpError("initial value expected to be of type ") << getType() << ", but was of type " << floatAttr.getType(); } - } else if (isa(initValue)) { - if (getConstant()) { - return emitOpError("cannot declare uninitialized constant"); - } } else { return emitOpError( "initial value should be a unit, integer, float or elements " "attribute, but got ") << initValue; } + } else if (isConstant() && !isExternal()) { + return emitOpError("cannot define uninitialized constant"); } return success(); } diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index 0c667bdfe875a..2c171b7a681ea 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -390,8 +390,8 @@ func.func @logical_or_resulterror(%arg0: i32, %arg1: i32) { // ----- -// expected-error @+1 {{'emitc.global' op cannot declare uninitialized constant}} -emitc.global constant @uninit : i32 = uninitialized +// expected-error @+1 {{'emitc.global' op cannot define uninitialized constant}} +emitc.global const @uninit : i32 // ----- diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir index d9e9ddd2f0496..7dbf16d5778d1 100644 --- a/mlir/test/Dialect/EmitC/ops.mlir +++ b/mlir/test/Dialect/EmitC/ops.mlir @@ -234,10 +234,10 @@ emitc.verbatim "typedef int32_t i32;" emitc.verbatim "typedef float f32;" -emitc.global @uninit : i32 = uninitialized +emitc.global @uninit : i32 emitc.global @myglobal_int : i32 = 4 emitc.global @myglobal : !emitc.array<2xf32> = dense<4.000000e+00> -emitc.global constant @myconstant : !emitc.array<2xi16> = dense<2> +emitc.global const @myconstant : !emitc.array<2xi16> = dense<2> func.func @use_global(%i: index) -> f32 { %0 = emitc.get_global @myglobal : !emitc.array<2xf32> diff --git a/mlir/test/Target/Cpp/global.mlir b/mlir/test/Target/Cpp/global.mlir index f67cac5c9f14e..e38509e0a6fe5 100644 --- a/mlir/test/Target/Cpp/global.mlir +++ b/mlir/test/Target/Cpp/global.mlir @@ -1,10 +1,10 @@ // RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s // RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s -emitc.global @decl : i8 +emitc.global extern @decl : i8 // CHECK: extern int8_t decl; -emitc.global @uninit : i32 = uninitialized +emitc.global @uninit : i32 // CHECK: int32_t uninit; emitc.global @myglobal_int : i32 = 4 @@ -13,9 +13,12 @@ emitc.global @myglobal_int : i32 = 4 emitc.global @myglobal : !emitc.array<2xf32> = dense<4.000000e+00> // CHECK: float myglobal[2] = {4.000000000e+00f, 4.000000000e+00f}; -emitc.global constant @myconstant : !emitc.array<2xi16> = dense<2> +emitc.global const @myconstant : !emitc.array<2xi16> = dense<2> // CHECK: const int16_t myconstant[2] = {2, 2}; +emitc.global extern const @extern_constant : !emitc.array<2xi16> +// CHECK: extern const int16_t extern_constant[2]; + func.func @use_global(%i: index) -> f32 { %0 = emitc.get_global @myglobal : !emitc.array<2xf32> %1 = emitc.subscript %0[%i] : <2xf32> From 51e4011ca80949980c7f9ed0eca6404f57d9caad Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 13:37:36 +0100 Subject: [PATCH 3/9] Update doc --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index d56c58836a2b7..0fd8984fe0c05 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -1022,12 +1022,10 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { The `emitc.global` operation declares or defines a named global variable. The backing memory for the variable is allocated statically and is described by the type of the variable. - The operation is a declaration if no `initial_value` is - specified, else it is a definition. The `initial_value` can either be a unit - attribute to represent a definition of an uninitialized global variable, or - an elements attribute to represent the definition of a global variable with - an initial value. The global variable can also be marked constant using the - `constant` unit attribute. Writing to such constant global variables is + Optionally, and `initial_value` can be provided. + External linkage can be specified using the `extern` unit attribute. + The global variable can also be marked constant using the + `const` unit attribute. Writing to such constant global variables is undefined. The global variable can be accessed by using the `emitc.get_global` to From a279ddf206b7f5613bdd51ee08c25d0130e2f448 Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 14:31:35 +0100 Subject: [PATCH 4/9] Support internal linkage --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 13 ++++++++----- mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp | 6 ++++-- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 5 ++++- mlir/lib/Target/Cpp/TranslateToCpp.cpp | 4 +++- mlir/test/Dialect/EmitC/invalid_ops.mlir | 5 +++++ mlir/test/Dialect/EmitC/ops.mlir | 2 ++ mlir/test/Target/Cpp/global.mlir | 4 ++++ 7 files changed, 30 insertions(+), 9 deletions(-) diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index 0fd8984fe0c05..be83160dab1a1 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -1023,7 +1023,8 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { The backing memory for the variable is allocated statically and is described by the type of the variable. Optionally, and `initial_value` can be provided. - External linkage can be specified using the `extern` unit attribute. + Internal linkage can be specified using the `internal` unit attribute + and external linkage can be specified using the `external` unit attribute. The global variable can also be marked constant using the `const` unit attribute. Writing to such constant global variables is undefined. @@ -1038,8 +1039,8 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { emitc.global @x : emitc.array<2xf32> = dense<0.0, 2.0> // External global variable emitc.global extern @x : emitc.array<2xf32> - // Constant global variable - emitc.global const @x : i32 = 0 + // Constant global variable with internal linkage + emitc.global static const @x : i32 = 0 ``` }]; @@ -1047,15 +1048,17 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { TypeAttr:$type, OptionalAttr:$initial_value, UnitAttr:$external, + UnitAttr:$internal, UnitAttr:$constant); let assemblyFormat = [{ - (`extern` $external^)? (`const` $constant^)? + (`extern` $external^)? (`static` $internal^)? (`const` $constant^)? $sym_name `:` custom($type, $initial_value) attr-dict }]; let extraClassDeclaration = [{ - bool isExternal() { return getExternal(); } + bool hasExternalLinkage() { return getExternal(); } + bool hasInternalLinkage() { return getInternal(); } bool isConstant() { return getConstant(); } }]; let hasVerifier = 1; diff --git a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp index a9c625f8050cc..3b1862a48d8a6 100644 --- a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp +++ b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp @@ -126,16 +126,18 @@ struct ConvertGlobal final : public OpConversionPattern { op.getLoc(), "global variable with alignment requirement is " "currently not supported"); } - auto resultTy = getTypeConverter()->convertType(op.getType()); if (!resultTy) { return rewriter.notifyMatchFailure(op.getLoc(), "cannot convert result type"); } + bool staticLinkage = + SymbolTable::getSymbolVisibility(op) != SymbolTable::Visibility::Public; + rewriter.replaceOpWithNewOp( op, operands.getSymName(), resultTy, operands.getInitialValueAttr(), - operands.getConstant()); + /*external=*/false, staticLinkage, operands.getConstant()); return success(); } }; diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index 6bb3c42daa301..a9c2fb281daa5 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -965,9 +965,12 @@ LogicalResult GlobalOp::verify() { "attribute, but got ") << initValue; } - } else if (isConstant() && !isExternal()) { + } else if (isConstant() && !hasExternalLinkage()) { return emitOpError("cannot define uninitialized constant"); } + if (hasExternalLinkage() && hasInternalLinkage()) { + return emitOpError("cannot have internal and external linkage"); + } return success(); } diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index ff9b52d47ca99..bf97fe514c3f8 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -1403,8 +1403,10 @@ LogicalResult CppEmitter::emitVariableDeclaration(OpResult result, } LogicalResult CppEmitter::emitGlobalVariable(GlobalOp op) { - if (op.isExternal()) + if (op.hasExternalLinkage()) os << "extern "; + else if (op.hasInternalLinkage()) + os << "static "; if (op.getConstant()) os << "const "; diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index 2c171b7a681ea..4b3806f612714 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -395,6 +395,11 @@ emitc.global const @uninit : i32 // ----- +// expected-error @+1 {{'emitc.global' op cannot have internal and external linkage}} +emitc.global extern static @uninit : i32 + +// ----- + emitc.global @myglobal : !emitc.array<2xf32> func.func @use_global(%i: index) { diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir index 7dbf16d5778d1..b47002c78b033 100644 --- a/mlir/test/Dialect/EmitC/ops.mlir +++ b/mlir/test/Dialect/EmitC/ops.mlir @@ -236,6 +236,8 @@ emitc.verbatim "typedef float f32;" emitc.global @uninit : i32 emitc.global @myglobal_int : i32 = 4 +emitc.global extern @external_linkage : i32 +emitc.global static @internal_linkage : i32 emitc.global @myglobal : !emitc.array<2xf32> = dense<4.000000e+00> emitc.global const @myconstant : !emitc.array<2xi16> = dense<2> diff --git a/mlir/test/Target/Cpp/global.mlir b/mlir/test/Target/Cpp/global.mlir index e38509e0a6fe5..1f5e55816d19b 100644 --- a/mlir/test/Target/Cpp/global.mlir +++ b/mlir/test/Target/Cpp/global.mlir @@ -19,6 +19,10 @@ emitc.global const @myconstant : !emitc.array<2xi16> = dense<2> emitc.global extern const @extern_constant : !emitc.array<2xi16> // CHECK: extern const int16_t extern_constant[2]; + +emitc.global static @internal_linkage : f32 +// CHECK: static float internal_linkage; + func.func @use_global(%i: index) -> f32 { %0 = emitc.get_global @myglobal : !emitc.array<2xf32> %1 = emitc.subscript %0[%i] : <2xf32> From f7b8cc9f238ce504d3dc1931d6156adce7fd3d1b Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 14:38:48 +0100 Subject: [PATCH 5/9] Update test --- .../Conversion/MemRefToEmitC/memref-to-emitc.mlir | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir index 504436183a4fe..4deb8dfd59dca 100644 --- a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir +++ b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir @@ -41,7 +41,7 @@ func.func @memref_load_store(%in: memref<4x8xf32>, %out: memref<3x5xf32>, %i: in // CHECK-LABEL: alloca func.func @alloca() { - // CHECK "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> !emitc.array<4x8xf32> + // CHECK: "emitc.variable"() <{value = #emitc.opaque<"">}> : () -> !emitc.array<4x8xf32> %0 = memref.alloca() : memref<4x8xf32> return } @@ -49,11 +49,14 @@ func.func @alloca() { // CHECK-LABEL: globals module @globals { - memref.global "private" constant @myglobal : memref<3x7xf32> = dense<4.0> - // CHECK emitc.global constant @myglobal : !emitc.array<3x7xf32> = dense<4.000000e+00> + memref.global "private" constant @internal_global : memref<3x7xf32> = dense<4.0> + // CHECK: emitc.global static const @internal_global : !emitc.array<3x7xf32> = dense<4.000000e+00> + memref.global @public_global : memref<3x7xf32> + // CHECK: emitc.global @public_global : !emitc.array<3x7xf32> + func.func @use_global() { - // CHECK emitc.get_global @myglobal : !emitc.array<3x7xf32> - %0 = memref.get_global @myglobal : memref<3x7xf32> + // CHECK: emitc.get_global @public_global : !emitc.array<3x7xf32> + %0 = memref.get_global @public_global : memref<3x7xf32> return } } From 3714794b362c433e27d5e0bbfaea789293103b78 Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 15:05:16 +0100 Subject: [PATCH 6/9] Rework linkage to specifiers --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 14 +++++++------- .../lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp | 7 +++++-- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 6 ++---- mlir/lib/Target/Cpp/TranslateToCpp.cpp | 4 ++-- .../Conversion/MemRefToEmitC/memref-to-emitc.mlir | 2 +- mlir/test/Dialect/EmitC/invalid_ops.mlir | 7 +------ 6 files changed, 18 insertions(+), 22 deletions(-) diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index be83160dab1a1..02b83a59c7665 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -1023,8 +1023,10 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { The backing memory for the variable is allocated statically and is described by the type of the variable. Optionally, and `initial_value` can be provided. - Internal linkage can be specified using the `internal` unit attribute - and external linkage can be specified using the `external` unit attribute. + Internal linkage can be specified using the `staticSpecifier` unit attribute + and external linkage can be specified using the `externSpecifier` unit attribute. + Note that the default linkage without those two keywords depends on whether + the target is C or C++ and whether the global variable is `const`. The global variable can also be marked constant using the `const` unit attribute. Writing to such constant global variables is undefined. @@ -1047,18 +1049,16 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { let arguments = (ins SymbolNameAttr:$sym_name, TypeAttr:$type, OptionalAttr:$initial_value, - UnitAttr:$external, - UnitAttr:$internal, + UnitAttr:$externSpecifier, + UnitAttr:$staticSpecifier, UnitAttr:$constant); let assemblyFormat = [{ - (`extern` $external^)? (`static` $internal^)? (`const` $constant^)? + (`extern` $externSpecifier^)? (`static` $staticSpecifier^)? (`const` $constant^)? $sym_name `:` custom($type, $initial_value) attr-dict }]; let extraClassDeclaration = [{ - bool hasExternalLinkage() { return getExternal(); } - bool hasInternalLinkage() { return getInternal(); } bool isConstant() { return getConstant(); } }]; let hasVerifier = 1; diff --git a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp index 3b1862a48d8a6..759e4256878a7 100644 --- a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp +++ b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp @@ -132,12 +132,15 @@ struct ConvertGlobal final : public OpConversionPattern { "cannot convert result type"); } - bool staticLinkage = + // We are explicit in specifier the linkage because the default linkage + // for constants is different in C and C++. + bool staticSpecifier = SymbolTable::getSymbolVisibility(op) != SymbolTable::Visibility::Public; + bool externSpecifier = !staticSpecifier; rewriter.replaceOpWithNewOp( op, operands.getSymName(), resultTy, operands.getInitialValueAttr(), - /*external=*/false, staticLinkage, operands.getConstant()); + externSpecifier, staticSpecifier, operands.getConstant()); return success(); } }; diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index a9c2fb281daa5..217c392174fcb 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -965,11 +965,9 @@ LogicalResult GlobalOp::verify() { "attribute, but got ") << initValue; } - } else if (isConstant() && !hasExternalLinkage()) { - return emitOpError("cannot define uninitialized constant"); } - if (hasExternalLinkage() && hasInternalLinkage()) { - return emitOpError("cannot have internal and external linkage"); + if (getStaticSpecifier() && getExternSpecifier()) { + return emitOpError("cannot have both static and extern specifiers"); } return success(); } diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index bf97fe514c3f8..63a99347f6945 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -1403,9 +1403,9 @@ LogicalResult CppEmitter::emitVariableDeclaration(OpResult result, } LogicalResult CppEmitter::emitGlobalVariable(GlobalOp op) { - if (op.hasExternalLinkage()) + if (op.getExternSpecifier()) os << "extern "; - else if (op.hasInternalLinkage()) + else if (op.getStaticSpecifier()) os << "static "; if (op.getConstant()) os << "const "; diff --git a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir index 4deb8dfd59dca..95e7fb6d90f61 100644 --- a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir +++ b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc.mlir @@ -52,7 +52,7 @@ module @globals { memref.global "private" constant @internal_global : memref<3x7xf32> = dense<4.0> // CHECK: emitc.global static const @internal_global : !emitc.array<3x7xf32> = dense<4.000000e+00> memref.global @public_global : memref<3x7xf32> - // CHECK: emitc.global @public_global : !emitc.array<3x7xf32> + // CHECK: emitc.global extern @public_global : !emitc.array<3x7xf32> func.func @use_global() { // CHECK: emitc.get_global @public_global : !emitc.array<3x7xf32> diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index 4b3806f612714..b5cf8f93da68b 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -390,12 +390,7 @@ func.func @logical_or_resulterror(%arg0: i32, %arg1: i32) { // ----- -// expected-error @+1 {{'emitc.global' op cannot define uninitialized constant}} -emitc.global const @uninit : i32 - -// ----- - -// expected-error @+1 {{'emitc.global' op cannot have internal and external linkage}} +// expected-error @+1 {{'emitc.global' op cannot have both static and extern specifiers}} emitc.global extern static @uninit : i32 // ----- From 5e78282a11d6a783a4635ad8346e9f4020adef77 Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 15:13:39 +0100 Subject: [PATCH 7/9] review comments --- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 3 +-- mlir/test/Dialect/EmitC/invalid_ops.mlir | 2 +- mlir/test/Target/Cpp/global.mlir | 11 +++++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index 217c392174fcb..db6a5dd51bc60 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -978,8 +978,7 @@ LogicalResult GlobalOp::verify() { LogicalResult GetGlobalOp::verifySymbolUses(SymbolTableCollection &symbolTable) { - // Verify that the result type is same as the type of the referenced - // memref.global op. + // Verify that the type matches the type of the global variable. auto global = symbolTable.lookupNearestSymbolFrom(*this, getNameAttr()); if (!global) diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index b5cf8f93da68b..437bf7699b736 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -397,7 +397,7 @@ emitc.global extern static @uninit : i32 emitc.global @myglobal : !emitc.array<2xf32> -func.func @use_global(%i: index) { +func.func @use_global() { // expected-error @+1 {{'emitc.get_global' op result type 'f32' does not match type '!emitc.array<2xf32>' of the global @myglobal}} %0 = emitc.get_global @myglobal : f32 return diff --git a/mlir/test/Target/Cpp/global.mlir b/mlir/test/Target/Cpp/global.mlir index 1f5e55816d19b..66c1ca0acd5b1 100644 --- a/mlir/test/Target/Cpp/global.mlir +++ b/mlir/test/Target/Cpp/global.mlir @@ -19,14 +19,17 @@ emitc.global const @myconstant : !emitc.array<2xi16> = dense<2> emitc.global extern const @extern_constant : !emitc.array<2xi16> // CHECK: extern const int16_t extern_constant[2]; +emitc.global static @static_var : f32 +// CHECK: static float static_var; -emitc.global static @internal_linkage : f32 -// CHECK: static float internal_linkage; +emitc.global static @static_const : f32 = 3.0 +// CHECK: static float static_const = 3.000000000e+00f; func.func @use_global(%i: index) -> f32 { %0 = emitc.get_global @myglobal : !emitc.array<2xf32> %1 = emitc.subscript %0[%i] : <2xf32> return %1 : f32 - // CHECK: float use_global(size_t v1) { - // CHECK: return myglobal[v1]; + // CHECK-LABEL: use_global + // CHECK-SAME: (size_t [[V1:.*]]) + // CHECK: return myglobal[[[V1]]]; } From 07e8c2f18506ba2f396810145412c2adeab56fb9 Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 15:22:34 +0100 Subject: [PATCH 8/9] bail on nested --- mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp | 9 +++++++-- .../Conversion/MemRefToEmitC/memref-to-emitc-failed.mlir | 5 +++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp index ce64bf9fd9d1d..ce27e93ccd8f1 100644 --- a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp +++ b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp @@ -74,10 +74,15 @@ struct ConvertGlobal final : public OpConversionPattern { "cannot convert result type"); } + SymbolTable::Visibility visibility = SymbolTable::getSymbolVisibility(op); + if (visibility == SymbolTable::Visibility::Nested) { + return rewriter.notifyMatchFailure(op.getLoc(), + "nested visibility is " + "currently not supported"); + } // We are explicit in specifier the linkage because the default linkage // for constants is different in C and C++. - bool staticSpecifier = - SymbolTable::getSymbolVisibility(op) != SymbolTable::Visibility::Public; + bool staticSpecifier = visibility == SymbolTable::Visibility::Private; bool externSpecifier = !staticSpecifier; rewriter.replaceOpWithNewOp( diff --git a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc-failed.mlir b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc-failed.mlir index 390190d341e5a..89dafa7529ed5 100644 --- a/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc-failed.mlir +++ b/mlir/test/Conversion/MemRefToEmitC/memref-to-emitc-failed.mlir @@ -38,3 +38,8 @@ func.func @zero_rank() { %0 = memref.alloca() : memref return } + +// ----- + +// expected-error@+1 {{failed to legalize operation 'memref.global'}} +memref.global "nested" constant @nested_global : memref<3x7xf32> From 0e5678ba8c5799a59a193382c70d8bad8c8d3452 Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Mon, 25 Mar 2024 16:37:11 +0100 Subject: [PATCH 9/9] review comments --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 17 +++++++++-------- .../Conversion/MemRefToEmitC/MemRefToEmitC.cpp | 9 +++++---- mlir/lib/Target/Cpp/TranslateToCpp.cpp | 2 +- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index dbd67c24158a5..ee5fc0b09a161 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -1027,8 +1027,8 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { and external linkage can be specified using the `externSpecifier` unit attribute. Note that the default linkage without those two keywords depends on whether the target is C or C++ and whether the global variable is `const`. - The global variable can also be marked constant using the - `const` unit attribute. Writing to such constant global variables is + The global variable can also be marked constant using the `constSpecifier` + unit attribute. Writing to such constant global variables is undefined. The global variable can be accessed by using the `emitc.get_global` to @@ -1051,16 +1051,17 @@ def EmitC_GlobalOp : EmitC_Op<"global", [Symbol]> { OptionalAttr:$initial_value, UnitAttr:$externSpecifier, UnitAttr:$staticSpecifier, - UnitAttr:$constant); + UnitAttr:$constSpecifier); let assemblyFormat = [{ - (`extern` $externSpecifier^)? (`static` $staticSpecifier^)? (`const` $constant^)? - $sym_name `:` custom($type, $initial_value) attr-dict + (`extern` $externSpecifier^)? + (`static` $staticSpecifier^)? + (`const` $constSpecifier^)? + $sym_name + `:` custom($type, $initial_value) + attr-dict }]; - let extraClassDeclaration = [{ - bool isConstant() { return getConstant(); } - }]; let hasVerifier = 1; } diff --git a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp index ce27e93ccd8f1..d3e7f233c0841 100644 --- a/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp +++ b/mlir/lib/Conversion/MemRefToEmitC/MemRefToEmitC.cpp @@ -75,10 +75,11 @@ struct ConvertGlobal final : public OpConversionPattern { } SymbolTable::Visibility visibility = SymbolTable::getSymbolVisibility(op); - if (visibility == SymbolTable::Visibility::Nested) { - return rewriter.notifyMatchFailure(op.getLoc(), - "nested visibility is " - "currently not supported"); + if (visibility != SymbolTable::Visibility::Public && + visibility != SymbolTable::Visibility::Private) { + return rewriter.notifyMatchFailure( + op.getLoc(), + "only public and private visibility is currently supported"); } // We are explicit in specifier the linkage because the default linkage // for constants is different in C and C++. diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index 678f8c7c96cbb..820bb65dff0ac 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -1409,7 +1409,7 @@ LogicalResult CppEmitter::emitGlobalVariable(GlobalOp op) { os << "extern "; else if (op.getStaticSpecifier()) os << "static "; - if (op.getConstant()) + if (op.getConstSpecifier()) os << "const "; if (failed(emitVariableDeclaration(op->getLoc(), op.getType(),