diff --git a/flang/include/flang/Optimizer/Dialect/CUF/CUFDialect.td b/flang/include/flang/Optimizer/Dialect/CUF/CUFDialect.td index df866e566406..0cabd55f1ea5 100644 --- a/flang/include/flang/Optimizer/Dialect/CUF/CUFDialect.td +++ b/flang/include/flang/Optimizer/Dialect/CUF/CUFDialect.td @@ -29,7 +29,6 @@ def CUFDialect : Dialect { }]; let useDefaultAttributePrinterParser = 1; - let usePropertiesForAttributes = 1; let cppNamespace = "::cuf"; let dependentDialects = ["fir::FIROpsDialect"]; diff --git a/flang/include/flang/Optimizer/Dialect/FIRCG/CGOps.td b/flang/include/flang/Optimizer/Dialect/FIRCG/CGOps.td index c05b03bd63ea..f3c72eaf4495 100644 --- a/flang/include/flang/Optimizer/Dialect/FIRCG/CGOps.td +++ b/flang/include/flang/Optimizer/Dialect/FIRCG/CGOps.td @@ -22,7 +22,6 @@ include "mlir/IR/BuiltinAttributes.td" def fircg_Dialect : Dialect { let name = "fircg"; let cppNamespace = "::fir::cg"; - let usePropertiesForAttributes = 1; } // Base class for FIR CG operations. diff --git a/flang/include/flang/Optimizer/Dialect/FIRDialect.td b/flang/include/flang/Optimizer/Dialect/FIRDialect.td index b05f4e731bc7..2ead7348cd4e 100644 --- a/flang/include/flang/Optimizer/Dialect/FIRDialect.td +++ b/flang/include/flang/Optimizer/Dialect/FIRDialect.td @@ -26,7 +26,6 @@ def FIROpsDialect : Dialect { let cppNamespace = "::fir"; let useDefaultTypePrinterParser = 0; let useDefaultAttributePrinterParser = 0; - let usePropertiesForAttributes = 1; let dependentDialects = [ // Arith dialect provides FastMathFlagsAttr // supported by some FIR operations. diff --git a/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td b/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td index 0bddfd85d436..1f46b69561d6 100644 --- a/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td +++ b/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td @@ -39,7 +39,6 @@ def hlfir_Dialect : Dialect { }]; let useDefaultTypePrinterParser = 1; - let usePropertiesForAttributes = 1; let cppNamespace = "hlfir"; let dependentDialects = ["fir::FIROpsDialect"]; } diff --git a/mlir/include/mlir/IR/DialectBase.td b/mlir/include/mlir/IR/DialectBase.td index 115a01f92706..efa09a43ec58 100644 --- a/mlir/include/mlir/IR/DialectBase.td +++ b/mlir/include/mlir/IR/DialectBase.td @@ -92,9 +92,6 @@ class Dialect { // If this dialect can be extended at runtime with new operations or types. bit isExtensible = 0; - - // Whether inherent Attributes defined in ODS will be stored as Properties. - bit usePropertiesForAttributes = 1; } #endif // DIALECTBASE_TD diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h index 79b929ff85e5..18f5074677cf 100644 --- a/mlir/include/mlir/IR/OperationSupport.h +++ b/mlir/include/mlir/IR/OperationSupport.h @@ -570,9 +570,7 @@ public: return ConcreteOp::getInherentAttr(concreteOp->getContext(), concreteOp.getProperties(), name); } - // If the op does not have support for properties, we dispatch back to the - // dictionnary of discardable attributes for now. - return cast(op)->getDiscardableAttr(name); + return std::nullopt; } void setInherentAttr(Operation *op, StringAttr name, Attribute value) final { @@ -581,9 +579,8 @@ public: return ConcreteOp::setInherentAttr(concreteOp.getProperties(), name, value); } - // If the op does not have support for properties, we dispatch back to the - // dictionnary of discardable attributes for now. - return cast(op)->setDiscardableAttr(name, value); + llvm_unreachable( + "Can't call setInherentAttr on operation with empty properties"); } void populateInherentAttrs(Operation *op, NamedAttrList &attrs) final { if constexpr (hasProperties) { @@ -639,7 +636,7 @@ public: auto p = properties.as(); return ConcreteOp::setPropertiesFromAttr(*p, attr, emitError); } - emitError() << "this operation does not support properties"; + emitError() << "this operation has empty properties"; return failure(); } Attribute getPropertiesAsAttr(Operation *op) final { @@ -651,11 +648,9 @@ public: return {}; } bool compareProperties(OpaqueProperties lhs, OpaqueProperties rhs) final { - if constexpr (hasProperties) { + if constexpr (hasProperties) return *lhs.as() == *rhs.as(); - } else { - return true; - } + return true; } void copyProperties(OpaqueProperties lhs, OpaqueProperties rhs) final { *lhs.as() = *rhs.as(); diff --git a/mlir/include/mlir/TableGen/Dialect.h b/mlir/include/mlir/TableGen/Dialect.h index 37c6427a9282..30f9d690b678 100644 --- a/mlir/include/mlir/TableGen/Dialect.h +++ b/mlir/include/mlir/TableGen/Dialect.h @@ -88,10 +88,6 @@ public: /// operations or types. bool isExtensible() const; - /// Default to use properties for storing Attributes for operations in this - /// dialect. - bool usePropertiesForAttributes() const; - const llvm::DagInit *getDiscardableAttributes() const; const llvm::Record *getDef() const { return def; } diff --git a/mlir/lib/TableGen/Dialect.cpp b/mlir/lib/TableGen/Dialect.cpp index ef39818e439b..7aaf4dd57c50 100644 --- a/mlir/lib/TableGen/Dialect.cpp +++ b/mlir/lib/TableGen/Dialect.cpp @@ -102,10 +102,6 @@ bool Dialect::isExtensible() const { return def->getValueAsBit("isExtensible"); } -bool Dialect::usePropertiesForAttributes() const { - return def->getValueAsBit("usePropertiesForAttributes"); -} - const llvm::DagInit *Dialect::getDiscardableAttributes() const { return def->getValueAsDag("discardableAttrs"); } diff --git a/mlir/test/mlir-tblgen/op-attribute.td b/mlir/test/mlir-tblgen/op-attribute.td index 4107c1fcc26c..cfdaaebb9e91 100644 --- a/mlir/test/mlir-tblgen/op-attribute.td +++ b/mlir/test/mlir-tblgen/op-attribute.td @@ -469,53 +469,6 @@ def EOp : NS_Op<"e_op", []> { // DECL-LABEL: EOp declarations // DECL: static void build({{.*}}, uint32_t i32_attr, uint32_t dv_i32_attr, ::llvm::APFloat f64_attr, ::llvm::APFloat dv_f64_attr, ::llvm::StringRef str_attr, ::llvm::StringRef dv_str_attr, bool bool_attr, bool dv_bool_attr, ::SomeI32Enum enum_attr, ::SomeI32Enum dv_enum_attr = ::SomeI32Enum::case5) -// Test verifyInvariantsImpl for op in dialect with `usePropertiesForAttributes = 0` set. -// --- - -def Test3_Dialect : Dialect { - let name = "test3"; - let cppNamespace = "foobar3"; - let usePropertiesForAttributes = 0; -} -def FOp : Op { - let arguments = (ins - SomeAttr:$aAttr, - DefaultValuedOptionalAttr:$bAttr, - OptionalAttr:$cAttr - ); -} - -// DEF-LABEL: FOp definitions -// DEF: ::llvm::LogicalResult FOp::verifyInvariantsImpl() { -// DEF: auto namedAttrRange = (*this)->getAttrs(); -// DEF: auto namedAttrIt = namedAttrRange.begin(); -// DEF: ::mlir::Attribute tblgen_aAttr; - -// DEF: while (true) { -// DEF: if (namedAttrIt == namedAttrRange.end()) -// DEF: return emitOpError("requires attribute 'aAttr'"); -// DEF: if (namedAttrIt->getName() == getAAttrAttrName()) { -// DEF: tblgen_aAttr = namedAttrIt->getValue(); -// DEF: break; -// DEF: } -// DEF: ++namedAttrIt; -// DEF: } - -// DEF: ::mlir::Attribute tblgen_bAttr; -// DEF: ::mlir::Attribute tblgen_cAttr; -// DEF: while (true) { -// DEF: if (namedAttrIt == namedAttrRange.end()) { -// DEF: break; -// DEF: } -// DEF: else if (namedAttrIt->getName() == getBAttrAttrName()) { -// DEF: tblgen_bAttr = namedAttrIt->getValue(); -// DEF: } -// DEF: else if (namedAttrIt->getName() == getCAttrAttrName()) { -// DEF: tblgen_cAttr = namedAttrIt->getValue(); -// DEF: } -// DEF: ++namedAttrIt; -// DEF: } - // Test proper namespacing for AttrDef // --- diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp index d1f1e8537113..e93f91bb540c 100644 --- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp +++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp @@ -60,6 +60,8 @@ static const char *const tblgenNamePrefix = "tblgen_"; static const char *const generatedArgName = "odsArg"; static const char *const odsBuilder = "odsBuilder"; static const char *const builderOpState = "odsState"; +static const char *const builderOpStateProperties = + "odsState.getOrAddProperties()"; static const char *const propertyStorage = "propStorage"; static const char *const propertyValue = "propValue"; static const char *const propertyAttr = "propAttr"; @@ -123,21 +125,10 @@ static const char *const attrSizedSegmentValueRangeCalcCode = R"( /// The code snippet to initialize the sizes for the value range calculation. /// /// {0}: The code to get the attribute. -static const char *const adapterSegmentSizeAttrInitCode = R"( - assert({0} && "missing segment size attribute for op"); - auto sizeAttr = ::llvm::cast<::mlir::DenseI32ArrayAttr>({0}); -)"; static const char *const adapterSegmentSizeAttrInitCodeProperties = R"( ::llvm::ArrayRef sizeAttr = {0}; )"; -/// The code snippet to initialize the sizes for the value range calculation. -/// -/// {0}: The code to get the attribute. -static const char *const opSegmentSizeAttrInitCode = R"( - auto sizeAttr = ::llvm::cast<::mlir::DenseI32ArrayAttr>({0}); -)"; - /// The logic to calculate the actual value range for a declared operand /// of an op with variadic of variadic operands within the OpAdaptor. /// @@ -431,8 +422,6 @@ public: bool hasProperties() const { if (!op.getProperties().empty()) return true; - if (!op.getDialect().usePropertiesForAttributes()) - return false; return true; } @@ -537,36 +526,22 @@ void OpOrAdaptorHelper::computeAttrMetadata() { }; // Include key attributes from several traits as implicitly registered. if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) { - if (op.getDialect().usePropertiesForAttributes()) { - operandSegmentsSizeStorage = - llvm::formatv("std::array", op.getNumOperands()); - operandSegmentsSizeParser = - llvm::formatv(parseTextualSegmentSizeFormat, op.getNumOperands()); - operandSegmentsSize = { - "operandSegmentSizes", - makeProperty(operandSegmentsSizeStorage, operandSegmentsSizeParser)}; - } else { - attrMetadata.insert( - {operandSegmentAttrName, AttributeMetadata{operandSegmentAttrName, - /*isRequired=*/true, - /*attr=*/std::nullopt}}); - } + operandSegmentsSizeStorage = + llvm::formatv("std::array", op.getNumOperands()); + operandSegmentsSizeParser = + llvm::formatv(parseTextualSegmentSizeFormat, op.getNumOperands()); + operandSegmentsSize = { + "operandSegmentSizes", + makeProperty(operandSegmentsSizeStorage, operandSegmentsSizeParser)}; } if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments")) { - if (op.getDialect().usePropertiesForAttributes()) { - resultSegmentsSizeStorage = - llvm::formatv("std::array", op.getNumResults()); - resultSegmentsSizeParser = - llvm::formatv(parseTextualSegmentSizeFormat, op.getNumResults()); - resultSegmentsSize = { - "resultSegmentSizes", - makeProperty(resultSegmentsSizeStorage, resultSegmentsSizeParser)}; - } else { - attrMetadata.insert( - {resultSegmentAttrName, - AttributeMetadata{resultSegmentAttrName, /*isRequired=*/true, - /*attr=*/std::nullopt}}); - } + resultSegmentsSizeStorage = + llvm::formatv("std::array", op.getNumResults()); + resultSegmentsSizeParser = + llvm::formatv(parseTextualSegmentSizeFormat, op.getNumResults()); + resultSegmentsSize = { + "resultSegmentSizes", + makeProperty(resultSegmentsSizeStorage, resultSegmentsSizeParser)}; } // Store the metadata in sorted order. @@ -865,43 +840,6 @@ static void populateSubstitutions(const OpOrAdaptorHelper &emitHelper, } } -/// Generate verification on native traits requiring attributes. -static void genNativeTraitAttrVerifier(MethodBody &body, - const OpOrAdaptorHelper &emitHelper) { - // Check that the variadic segment sizes attribute exists and contains the - // expected number of elements. - // - // {0}: Attribute name. - // {1}: Expected number of elements. - // {2}: "operand" or "result". - // {3}: Emit error prefix. - const char *const checkAttrSizedValueSegmentsCode = R"( - { - auto sizeAttr = ::llvm::cast<::mlir::DenseI32ArrayAttr>(tblgen_{0}); - auto numElements = sizeAttr.asArrayRef().size(); - if (numElements != {1}) - return {3}"'{0}' attribute for specifying {2} segments must have {1} " - "elements, but got ") << numElements; - } - )"; - - // Verify a few traits first so that we can use getODSOperands() and - // getODSResults() in the rest of the verifier. - auto &op = emitHelper.getOp(); - if (!op.getDialect().usePropertiesForAttributes()) { - if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) { - body << formatv(checkAttrSizedValueSegmentsCode, operandSegmentAttrName, - op.getNumOperands(), "operand", - emitHelper.emitErrorPrefix()); - } - if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments")) { - body << formatv(checkAttrSizedValueSegmentsCode, resultSegmentAttrName, - op.getNumResults(), "result", - emitHelper.emitErrorPrefix()); - } - } -} - // Return true if a verifier can be emitted for the attribute: it is not a // derived attribute, it has a predicate, its condition is not empty, and, for // adaptors, the condition does not reference the op. @@ -1077,10 +1015,6 @@ while (true) {{ } body.unindent(); - // Emit the checks for segment attributes first so that the other - // constraints can call operand and result getters. - genNativeTraitAttrVerifier(body, emitHelper); - bool isEmittingForOp = emitHelper.isEmittingForOp(); for (const auto &namedAttr : emitHelper.getOp().getAttributes()) if (canEmitAttrVerifier(namedAttr.attr, isEmittingForOp)) @@ -1275,7 +1209,6 @@ void OpEmitter::genAttrNameGetters() { const llvm::MapVector &attributes = emitHelper.getAttrMetadata(); bool hasOperandSegmentsSize = - op.getDialect().usePropertiesForAttributes() && op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments"); // Emit the getAttributeNames method. { @@ -2039,18 +1972,11 @@ void OpEmitter::genAttrGetters() { } void OpEmitter::genAttrSetters() { - bool useProperties = op.getDialect().usePropertiesForAttributes(); - // Generate the code to set an attribute. auto emitSetAttr = [&](Method *method, StringRef getterName, StringRef attrName, StringRef attrVar) { - if (useProperties) { - method->body() << formatv(" getProperties().{0} = {1};", attrName, - attrVar); - } else { - method->body() << formatv(" (*this)->setAttr({0}AttrName(), {1});", - getterName, attrVar); - } + method->body() << formatv(" getProperties().{0} = {1};", attrName, + attrVar); }; // Generate raw named setter type. This is a wrapper class that allows setting @@ -2110,25 +2036,15 @@ void OpEmitter::genAttrSetters() { // optional but not in the same way as the others (i.e. it uses bool over // std::optional<>). StringRef paramStr = isUnitAttr ? "attrValue" : "*attrValue"; - if (!useProperties) { - const char *optionalCodeBody = R"( - if (attrValue) - return (*this)->setAttr({0}AttrName(), {1}); - (*this)->removeAttr({0}AttrName());)"; - method->body() << formatv( - optionalCodeBody, getterName, - constBuildAttrFromParam(baseAttr, fctx, paramStr)); - } else { - const char *optionalCodeBody = R"( + const char *optionalCodeBody = R"( auto &odsProp = getProperties().{0}; if (attrValue) odsProp = {1}; else odsProp = nullptr;)"; - method->body() << formatv( - optionalCodeBody, attrName, - constBuildAttrFromParam(baseAttr, fctx, paramStr)); - } + method->body() << formatv( + optionalCodeBody, attrName, + constBuildAttrFromParam(baseAttr, fctx, paramStr)); }; for (const NamedAttribute &namedAttr : op.getAttributes()) { @@ -2146,28 +2062,22 @@ void OpEmitter::genAttrSetters() { void OpEmitter::genOptionalAttrRemovers() { // Generate methods for removing optional attributes, instead of having to // use the string interface. Enables better compile time verification. - auto emitRemoveAttr = [&](StringRef name, bool useProperties) { + auto emitRemoveAttr = [&](StringRef name) { auto *method = opClass.addInlineMethod("::mlir::Attribute", op.getRemoverName(name) + "Attr"); if (!method) return; - if (useProperties) { - method->body() << formatv(R"( + method->body() << formatv(R"( auto attr = getProperties().{0}; getProperties().{0} = {{}; return attr; )", - name); - return; - } - method->body() << formatv("return (*this)->removeAttr({0}AttrName());", - op.getGetterName(name)); + name); }; for (const NamedAttribute &namedAttr : op.getAttributes()) if (namedAttr.attr.isOptional()) - emitRemoveAttr(namedAttr.name, - op.getDialect().usePropertiesForAttributes()); + emitRemoveAttr(namedAttr.name); } // Generates the code to compute the start and end index of an operand or result @@ -2366,13 +2276,8 @@ void OpEmitter::genNamedOperandGetters() { // array. std::string attrSizeInitCode; if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) { - if (op.getDialect().usePropertiesForAttributes()) - attrSizeInitCode = formatv(adapterSegmentSizeAttrInitCodeProperties, - "getProperties().operandSegmentSizes"); - - else - attrSizeInitCode = formatv(opSegmentSizeAttrInitCode, - emitHelper.getAttr(operandSegmentAttrName)); + attrSizeInitCode = formatv(adapterSegmentSizeAttrInitCodeProperties, + "getProperties().operandSegmentSizes"); } generateNamedOperandGetters( @@ -2483,13 +2388,8 @@ void OpEmitter::genNamedResultGetters() { // Build the initializer string for the result segment size attribute. std::string attrSizeInitCode; if (attrSizedResults) { - if (op.getDialect().usePropertiesForAttributes()) - attrSizeInitCode = formatv(adapterSegmentSizeAttrInitCodeProperties, - "getProperties().resultSegmentSizes"); - - else - attrSizeInitCode = formatv(opSegmentSizeAttrInitCode, - emitHelper.getAttr(resultSegmentAttrName)); + attrSizeInitCode = formatv(adapterSegmentSizeAttrInitCodeProperties, + "getProperties().resultSegmentSizes"); } generateValueRangeStartAndEnd( @@ -2722,14 +2622,7 @@ void OpEmitter::genSeparateArgParamBuilder() { // Automatically create the 'resultSegmentSizes' attribute using // the length of the type ranges. if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments")) { - if (op.getDialect().usePropertiesForAttributes()) { - body << " ::llvm::copy(::llvm::ArrayRef({"; - } else { - std::string getterName = op.getGetterName(resultSegmentAttrName); - body << " " << builderOpState << ".addAttribute(" << getterName - << "AttrName(" << builderOpState << ".name), " - << "odsBuilder.getDenseI32ArrayAttr({"; - } + body << " ::llvm::copy(::llvm::ArrayRef({"; interleaveComma( llvm::seq(0, op.getNumResults()), body, [&](int i) { const NamedTypeConstraint &result = op.getResult(i); @@ -2746,13 +2639,8 @@ void OpEmitter::genSeparateArgParamBuilder() { body << "static_cast(" << resultNames[i] << ".size())"; } }); - if (op.getDialect().usePropertiesForAttributes()) { - body << "}), " << builderOpState - << ".getOrAddProperties()." - "resultSegmentSizes.begin());\n"; - } else { - body << "}));\n"; - } + body << "}), " << builderOpStateProperties + << ".resultSegmentSizes.begin());\n"; } return; @@ -3501,16 +3389,9 @@ void OpEmitter::genCodeForAddingArgAndRegionForBuilder( << " rangeSegments.push_back(range.size());\n" << " auto rangeAttr = " << odsBuilder << ".getDenseI32ArrayAttr(rangeSegments);\n"; - if (op.getDialect().usePropertiesForAttributes()) { - body << " " << builderOpState << ".getOrAddProperties()." - << operand.constraint.getVariadicOfVariadicSegmentSizeAttr() - << " = rangeAttr;"; - } else { - body << " " << builderOpState << ".addAttribute(" - << op.getGetterName( - operand.constraint.getVariadicOfVariadicSegmentSizeAttr()) - << "AttrName(" << builderOpState << ".name), rangeAttr);"; - } + body << " " << builderOpStateProperties << "." + << operand.constraint.getVariadicOfVariadicSegmentSizeAttr() + << " = rangeAttr;"; body << " }\n"; continue; } @@ -3545,19 +3426,10 @@ void OpEmitter::genCodeForAddingArgAndRegionForBuilder( }; if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) { std::string sizes = op.getGetterName(operandSegmentAttrName); - if (op.getDialect().usePropertiesForAttributes()) { - body << " ::llvm::copy(::llvm::ArrayRef({"; - emitSegment(); - body << "}), " << builderOpState - << ".getOrAddProperties()." - "operandSegmentSizes.begin());\n"; - } else { - body << " " << builderOpState << ".addAttribute(" << sizes << "AttrName(" - << builderOpState << ".name), " - << "odsBuilder.getDenseI32ArrayAttr({"; - emitSegment(); - body << "}));\n"; - } + body << " ::llvm::copy(::llvm::ArrayRef({"; + emitSegment(); + body << "}), " << builderOpStateProperties + << ".operandSegmentSizes.begin());\n"; } // Push all properties to the result. @@ -3566,8 +3438,8 @@ void OpEmitter::genCodeForAddingArgAndRegionForBuilder( // interface type (used in the builder argument) to the storage type (used // in the state) is not necessarily trivial. std::string setterName = op.getSetterName(namedProp.name); - body << formatv(" {0}.getOrAddProperties().{1}({2});\n", - builderOpState, setterName, namedProp.name); + body << formatv(" {0}.{1}({2});\n", builderOpStateProperties, setterName, + namedProp.name); } // Push all attributes to the result. for (const auto &namedAttr : op.getAttributes()) { @@ -3591,24 +3463,12 @@ void OpEmitter::genCodeForAddingArgAndRegionForBuilder( // instance. FmtContext fctx; fctx.withBuilder("odsBuilder"); - if (op.getDialect().usePropertiesForAttributes()) { - body << formatv(" {0}.getOrAddProperties().{1} = {2};\n", - builderOpState, namedAttr.name, - constBuildAttrFromParam(attr, fctx, namedAttr.name)); - } else { - body << formatv(" {0}.addAttribute({1}AttrName({0}.name), {2});\n", - builderOpState, op.getGetterName(namedAttr.name), - constBuildAttrFromParam(attr, fctx, namedAttr.name)); - } + body << formatv(" {0}.{1} = {2};\n", builderOpStateProperties, + namedAttr.name, + constBuildAttrFromParam(attr, fctx, namedAttr.name)); } else { - if (op.getDialect().usePropertiesForAttributes()) { - body << formatv(" {0}.getOrAddProperties().{1} = {1};\n", - builderOpState, namedAttr.name); - } else { - body << formatv(" {0}.addAttribute({1}AttrName({0}.name), {2});\n", - builderOpState, op.getGetterName(namedAttr.name), - namedAttr.name); - } + body << formatv(" {0}.{1} = {1};\n", builderOpStateProperties, + namedAttr.name); } if (emitNotNullCheck) body.unindent() << " }\n"; @@ -3935,20 +3795,12 @@ void OpEmitter::genTypeInterfaceMethods() { } else if (auto *attr = dyn_cast( op.getArg(arg.operandOrAttributeIndex()))) { body << " ::mlir::TypedAttr odsInferredTypeAttr" << inferredTypeIdx - << " = "; - if (op.getDialect().usePropertiesForAttributes()) { - body << "(properties ? properties.as()->" - << attr->name - << " : " - "::llvm::dyn_cast_or_null<::mlir::TypedAttr>(attributes." - "get(\"" + - attr->name + "\")));\n"; - } else { - body << "::llvm::dyn_cast_or_null<::mlir::TypedAttr>(attributes." - "get(\"" + - attr->name + "\"));\n"; - } - body << " if (!odsInferredTypeAttr" << inferredTypeIdx + << " = (properties ? properties.as()->" + << attr->name + << " : ::llvm::dyn_cast_or_null<::mlir::TypedAttr>(attributes." + "get(\"" + << attr->name << "\")));\n" + << " if (!odsInferredTypeAttr" << inferredTypeIdx << ") return ::mlir::failure();\n"; typeStr = ("odsInferredTypeAttr" + Twine(inferredTypeIdx) + ".getType()") @@ -4672,13 +4524,8 @@ OpOperandAdaptorEmitter::OpOperandAdaptorEmitter( std::string sizeAttrInit; if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) { - if (op.getDialect().usePropertiesForAttributes()) - sizeAttrInit = - formatv(adapterSegmentSizeAttrInitCodeProperties, - llvm::formatv("getProperties().operandSegmentSizes")); - else - sizeAttrInit = formatv(adapterSegmentSizeAttrInitCode, - emitHelper.getAttr(operandSegmentAttrName)); + sizeAttrInit = formatv(adapterSegmentSizeAttrInitCodeProperties, + "getProperties().operandSegmentSizes"); } generateNamedOperandGetters(op, genericAdaptor, /*genericAdaptorBase=*/&genericAdaptorBase, diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp index b834c6f8d3aa..ff51fb403ffc 100644 --- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp +++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp @@ -446,6 +446,11 @@ static bool shouldFormatSymbolNameAttr(const NamedAttribute *attr) { return attr->attr.getBaseAttr().getAttrDefName() == "SymbolNameAttr"; } +/// The code snippet used to get properties from the operation state. +/// {0}: The C++ class name of the operation. +const char *const getPropertiesCode = + "result.getOrAddProperties<{0}::Properties>()"; + /// The code snippet used to generate a parser call for an attribute. /// /// {0}: The name of the attribute. @@ -528,15 +533,16 @@ const char *const enumAttrParserCode = R"( /// The code snippet used to generate a parser call for a property. /// {0}: The name of the property -/// {1}: The C++ class name of the operation -/// {2}: The property's parser code with appropriate substitutions performed -/// {3}: The description of the expected property for the error message. +/// {1}: The property access expression +/// (result.getOrAddProperties()) {2}: The property's parser +/// code with appropriate substitutions performed {3}: The description of the +/// expected property for the error message. const char *const propertyParserCode = R"( auto {0}PropLoc = parser.getCurrentLocation(); auto {0}PropParseResult = [&](auto& propStorage) -> ::mlir::ParseResult {{ {2} return ::mlir::success(); - }(result.getOrAddProperties<{1}::Properties>().{0}); + }({1}.{0}); if (failed({0}PropParseResult)) {{ return parser.emitError({0}PropLoc, "invalid value for property {0}, expected {3}"); } @@ -544,13 +550,14 @@ const char *const propertyParserCode = R"( /// The code snippet used to generate a parser call for a property. /// {0}: The name of the property -/// {1}: The C++ class name of the operation -/// {2}: The property's parser code with appropriate substitutions performed +/// {1}: The property access expression +/// (result.getOrAddProperties()) {2}: The property's parser +/// code with appropriate substitutions performed const char *const optionalPropertyParserCode = R"( auto {0}PropParseResult = [&](auto& propStorage) -> ::mlir::OptionalParseResult {{ {2} return ::mlir::success(); - }(result.getOrAddProperties<{1}::Properties>().{0}); + }({1}.{0}); if ({0}PropParseResult.has_value() && failed(*{0}PropParseResult)) {{ return ::mlir::failure(); } @@ -965,7 +972,8 @@ static void genElementParserStorage(FormatElement *element, const Operator &op, } /// Generate the parser for a parameter to a custom directive. -static void genCustomParameterParser(FormatElement *param, MethodBody &body) { +static void genCustomParameterParser(FormatElement *param, MethodBody &body, + StringRef opCppClassName) { if (auto *attr = dyn_cast(param)) { body << attr->getVar()->name << "Attr"; } else if (isa(param)) { @@ -999,7 +1007,7 @@ static void genCustomParameterParser(FormatElement *param, MethodBody &body) { body << formatv("{0}Successor", name); } else if (auto *dir = dyn_cast(param)) { - genCustomParameterParser(dir->getArg(), body); + genCustomParameterParser(dir->getArg(), body, opCppClassName); } else if (auto *dir = dyn_cast(param)) { ArgumentLengthKind lengthKind; @@ -1020,8 +1028,8 @@ static void genCustomParameterParser(FormatElement *param, MethodBody &body) { body << tgfmt(string->getValue(), &ctx); } else if (auto *property = dyn_cast(param)) { - body << formatv("result.getOrAddProperties().{0}", - property->getVar()->name); + body << formatv(getPropertiesCode, opCppClassName) << "." + << property->getVar()->name; } else { llvm_unreachable("unknown custom directive parameter"); } @@ -1092,7 +1100,7 @@ static void genCustomDirectiveParser(CustomDirective *dir, MethodBody &body, body << " auto odsResult = parse" << dir->getName() << "(parser"; for (FormatElement *param : dir->getElements()) { body << ", "; - genCustomParameterParser(param, body); + genCustomParameterParser(param, body, opCppClassName); } body << ");\n"; @@ -1110,9 +1118,8 @@ static void genCustomDirectiveParser(CustomDirective *dir, MethodBody &body, if (var->attr.isOptional() || var->attr.hasDefaultValue()) body << formatv(" if ({0}Attr)\n ", var->name); if (useProperties) { - body << formatv( - " result.getOrAddProperties<{1}::Properties>().{0} = {0}Attr;\n", - var->name, opCppClassName); + std::string propAccess = formatv(getPropertiesCode, opCppClassName); + body << formatv(" {0}.{1} = {1}Attr;\n", propAccess, var->name); } else { body << formatv(" result.addAttribute(\"{0}\", {0}Attr);\n", var->name); @@ -1190,10 +1197,8 @@ static void genEnumAttrParser(const NamedAttribute *var, MethodBody &body, } std::string attrAssignment; if (useProperties) { - attrAssignment = - formatv(" " - "result.getOrAddProperties<{1}::Properties>().{0} = {0}Attr;", - var->name, opCppClassName); + std::string propAccess = formatv(getPropertiesCode, opCppClassName); + attrAssignment = formatv(" {0}.{1} = {1}Attr;", propAccess, var->name); } else { attrAssignment = formatv("result.addAttribute(\"{0}\", {0}Attr);", var->name); @@ -1217,11 +1222,12 @@ static void genPropertyParser(PropertyVariable *propVar, MethodBody &body, fmtContext.addSubst("_ctxt", "parser.getContext()"); fmtContext.addSubst("_storage", "propStorage"); + std::string propAccess = formatv(getPropertiesCode, opCppClassName); if (parseOptionally) { - body << formatv(optionalPropertyParserCode, name, opCppClassName, + body << formatv(optionalPropertyParserCode, name, propAccess, tgfmt(prop.getOptionalParserCall(), &fmtContext)); } else { - body << formatv(propertyParserCode, name, opCppClassName, + body << formatv(propertyParserCode, name, propAccess, tgfmt(prop.getParserCall(), &fmtContext), prop.getSummary()); } @@ -1265,10 +1271,9 @@ static void genAttrParser(AttributeVariable *attr, MethodBody &body, } } if (useProperties) { - body << formatv( - " if ({0}Attr) result.getOrAddProperties<{1}::Properties>().{0} = " - "{0}Attr;\n", - var->name, opCppClassName); + std::string propAccess = formatv(getPropertiesCode, opCppClassName); + body << formatv(" if ({0}Attr) {1}.{0} = {0}Attr;\n", var->name, + propAccess); } else { body << formatv( " if ({0}Attr) result.attributes.append(\"{0}\", {0}Attr);\n", @@ -1432,6 +1437,7 @@ void OperationFormat::genParser(Operator &op, OpClass &opClass) { void OperationFormat::genElementParser(FormatElement *element, MethodBody &body, FmtContext &attrTypeCtx, GenContext genCtx) { + std::string propAccess = formatv(getPropertiesCode, opCppClassName); /// Optional Group. if (auto *optional = dyn_cast(element)) { auto genElementParsers = [&](FormatElement *firstElement, @@ -1448,14 +1454,11 @@ void OperationFormat::genElementParser(FormatElement *element, MethodBody &body, // Add the anchor unit attribute or property to the operation state // or set the property to true. if (isa(anchorVar)) { - body << formatv( - " result.getOrAddProperties<{1}::Properties>().{0} = true;", - anchorVar->getName(), opCppClassName); + body << formatv(" {0}.{1} = true;", propAccess, + anchorVar->getName()); } else if (useProperties) { - body << formatv( - " result.getOrAddProperties<{1}::Properties>().{0} = " - "parser.getBuilder().getUnitAttr();", - anchorVar->getName(), opCppClassName); + body << formatv(" {0}.{1} = parser.getBuilder().getUnitAttr();", + propAccess, anchorVar->getName()); } else { body << " result.addAttribute(\"" << anchorVar->getName() << "\", parser.getBuilder().getUnitAttr());\n"; @@ -1551,14 +1554,11 @@ void OperationFormat::genElementParser(FormatElement *element, MethodBody &body, if (AttributeLikeVariable *unitVarElem = oilist->getUnitVariableParsingElement(pelement)) { if (isa(unitVarElem)) { - body << formatv( - " result.getOrAddProperties<{1}::Properties>().{0} = true;", - unitVarElem->getName(), opCppClassName); + body << formatv(" {0}.{1} = true;", propAccess, + unitVarElem->getName()); } else if (useProperties) { - body << formatv( - " result.getOrAddProperties<{1}::Properties>().{0} = " - "parser.getBuilder().getUnitAttr();", - unitVarElem->getName(), opCppClassName); + body << formatv(" {0}.{1} = parser.getBuilder().getUnitAttr();", + propAccess, unitVarElem->getName()); } else { body << " result.addAttribute(\"" << unitVarElem->getName() << "\", UnitAttr::get(parser.getContext()));\n"; @@ -1906,6 +1906,7 @@ void OperationFormat::genParserSuccessorResolution(Operator &op, void OperationFormat::genParserVariadicSegmentResolution(Operator &op, MethodBody &body) { + std::string propAccess = formatv(getPropertiesCode, op.getCppClassName()); if (!allOperands) { if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) { auto interleaveFn = [&](const NamedTypeConstraint &operand) { @@ -1915,38 +1916,18 @@ void OperationFormat::genParserVariadicSegmentResolution(Operator &op, else body << "1"; }; - if (op.getDialect().usePropertiesForAttributes()) { - body << "::llvm::copy(::llvm::ArrayRef({"; - llvm::interleaveComma(op.getOperands(), body, interleaveFn); - body << formatv("}), " - "result.getOrAddProperties<{0}::Properties>()." - "operandSegmentSizes.begin());\n", - op.getCppClassName()); - } else { - body << " result.addAttribute(\"operandSegmentSizes\", " - << "parser.getBuilder().getDenseI32ArrayAttr({"; - llvm::interleaveComma(op.getOperands(), body, interleaveFn); - body << "}));\n"; - } + body << "::llvm::copy(::llvm::ArrayRef({"; + llvm::interleaveComma(op.getOperands(), body, interleaveFn); + body << "}), " << propAccess << ".operandSegmentSizes.begin());\n"; } for (const NamedTypeConstraint &operand : op.getOperands()) { if (!operand.isVariadicOfVariadic()) continue; - if (op.getDialect().usePropertiesForAttributes()) { - body << formatv( - " result.getOrAddProperties<{0}::Properties>().{1} = " - "parser.getBuilder().getDenseI32ArrayAttr({2}OperandGroupSizes);\n", - op.getCppClassName(), - operand.constraint.getVariadicOfVariadicSegmentSizeAttr(), - operand.name); - } else { - body << formatv( - " result.addAttribute(\"{0}\", " - "parser.getBuilder().getDenseI32ArrayAttr({1}OperandGroupSizes));" - "\n", - operand.constraint.getVariadicOfVariadicSegmentSizeAttr(), - operand.name); - } + body << formatv( + " {0}.{1} = " + "parser.getBuilder().getDenseI32ArrayAttr({2}OperandGroupSizes);\n", + propAccess, operand.constraint.getVariadicOfVariadicSegmentSizeAttr(), + operand.name); } } @@ -1959,19 +1940,9 @@ void OperationFormat::genParserVariadicSegmentResolution(Operator &op, else body << "1"; }; - if (op.getDialect().usePropertiesForAttributes()) { - body << "::llvm::copy(::llvm::ArrayRef({"; - llvm::interleaveComma(op.getResults(), body, interleaveFn); - body << formatv("}), " - "result.getOrAddProperties<{0}::Properties>()." - "resultSegmentSizes.begin());\n", - op.getCppClassName()); - } else { - body << " result.addAttribute(\"resultSegmentSizes\", " - << "parser.getBuilder().getDenseI32ArrayAttr({"; - llvm::interleaveComma(op.getResults(), body, interleaveFn); - body << "}));\n"; - } + body << "::llvm::copy(::llvm::ArrayRef({"; + llvm::interleaveComma(op.getResults(), body, interleaveFn); + body << "}), " << propAccess << ".resultSegmentSizes.begin());\n"; } } diff --git a/mlir/tools/mlir-tblgen/RewriterGen.cpp b/mlir/tools/mlir-tblgen/RewriterGen.cpp index 08d6483e1f1b..e3043708a46d 100644 --- a/mlir/tools/mlir-tblgen/RewriterGen.cpp +++ b/mlir/tools/mlir-tblgen/RewriterGen.cpp @@ -892,15 +892,9 @@ void PatternEmitter::emitAttributeMatch(DagNode tree, StringRef castedName, const auto &attr = namedAttr->attr; os << "{\n"; - if (op.getDialect().usePropertiesForAttributes()) { - os.indent() << formatv( - "[[maybe_unused]] auto tblgen_attr = {0}.getProperties().{1}();\n", - castedName, op.getGetterName(namedAttr->name)); - } else { - os.indent() << formatv("[[maybe_unused]] auto tblgen_attr = " - "{0}->getAttrOfType<{1}>(\"{2}\");\n", - castedName, attr.getStorageType(), namedAttr->name); - } + os.indent() << formatv( + "[[maybe_unused]] auto tblgen_attr = {0}.getProperties().{1}();\n", + castedName, op.getGetterName(namedAttr->name)); // TODO: This should use getter method to avoid duplication. if (attr.hasDefaultValue()) { @@ -1606,7 +1600,6 @@ std::string PatternEmitter::handleOpCreation(DagNode tree, int resultIndex, LLVM_DEBUG(llvm::dbgs() << '\n'); Operator &resultOp = tree.getDialectOp(opMap); - bool useProperties = resultOp.getDialect().usePropertiesForAttributes(); auto numOpArgs = resultOp.getNumArgs(); auto numPatArgs = tree.getNumArgs(); @@ -1698,10 +1691,9 @@ std::string PatternEmitter::handleOpCreation(DagNode tree, int resultIndex, createAggregateLocalVarsForOpArgs(tree, childNodeNames, depth); // Then create the op. - os.scope("", "\n}\n").os - << formatv("{0} = {1}::create(rewriter, {2}, tblgen_values, {3});", - valuePackName, resultOp.getQualCppClassName(), locToUse, - useProperties ? "tblgen_props" : "tblgen_attrs"); + os.scope("", "\n}\n").os << formatv( + "{0} = {1}::create(rewriter, {2}, tblgen_values, {3});", valuePackName, + resultOp.getQualCppClassName(), locToUse, "tblgen_props"); return resultValue; } @@ -1760,7 +1752,7 @@ std::string PatternEmitter::handleOpCreation(DagNode tree, int resultIndex, os << formatv("{0} = {1}::create(rewriter, {2}, tblgen_types, " "tblgen_values, {3});\n", valuePackName, resultOp.getQualCppClassName(), locToUse, - useProperties ? "tblgen_props" : "tblgen_attrs"); + "tblgen_props"); os.unindent() << "}\n"; return resultValue; } @@ -1874,26 +1866,15 @@ void PatternEmitter::createAggregateLocalVarsForOpArgs( DagNode node, const ChildNodeIndexNameMap &childNodeNames, int depth) { Operator &resultOp = node.getDialectOp(opMap); - bool useProperties = resultOp.getDialect().usePropertiesForAttributes(); auto scope = os.scope(); os << formatv("::llvm::SmallVector<::mlir::Value, 4> " "tblgen_values; (void)tblgen_values;\n"); - if (useProperties) { - os << formatv("{0}::Properties tblgen_props; (void)tblgen_props;\n", - resultOp.getQualCppClassName()); - } else { - os << formatv("::llvm::SmallVector<::mlir::NamedAttribute, 4> " - "tblgen_attrs; (void)tblgen_attrs;\n"); - } + os << formatv("{0}::Properties tblgen_props; (void)tblgen_props;\n", + resultOp.getQualCppClassName()); - const char *setPropCmd = + const char *setterCmd = "tblgen_props.{0} = " "::llvm::dyn_cast_if_present({1});\n"; - const char *addAttrCmd = - "if (auto tmpAttr = {1}) {\n" - " tblgen_attrs.emplace_back(rewriter.getStringAttr(\"{0}\"), " - "tmpAttr);\n}\n"; - const char *setterCmd = (useProperties) ? setPropCmd : addAttrCmd; const char *propSetterCmd = "tblgen_props.{0}({1});\n"; int numVariadic = 0; @@ -1992,18 +1973,10 @@ void PatternEmitter::createAggregateLocalVarsForOpArgs( const auto *sameVariadicSize = resultOp.getTrait("::mlir::OpTrait::SameVariadicOperandSize"); if (!sameVariadicSize) { - if (useProperties) { - const char *setSizes = R"( + const char *setSizes = R"( tblgen_props.operandSegmentSizes = {{ {0} }; )"; - os.printReindented(formatv(setSizes, llvm::join(sizes, ", ")).str()); - } else { - const char *setSizes = R"( - tblgen_attrs.emplace_back(rewriter.getStringAttr("operandSegmentSizes"), - rewriter.getDenseI32ArrayAttr({{ {0} })); - )"; - os.printReindented(formatv(setSizes, llvm::join(sizes, ", ")).str()); - } + os.printReindented(formatv(setSizes, llvm::join(sizes, ", ")).str()); } } } diff --git a/mlir/unittests/Bytecode/BytecodeTest.cpp b/mlir/unittests/Bytecode/BytecodeTest.cpp index 30e7ed9b6cb7..148868801da3 100644 --- a/mlir/unittests/Bytecode/BytecodeTest.cpp +++ b/mlir/unittests/Bytecode/BytecodeTest.cpp @@ -223,8 +223,9 @@ TEST(Bytecode, OpWithoutProperties) { llvm::MemoryBufferRef(bytecode, "string-buffer"), block.get(), config))); Operation *roundtripped = &block->front(); EXPECT_EQ(roundtripped->getAttrs().size(), 2u); - EXPECT_TRUE(roundtripped->getInherentAttr("inherent_attr") != std::nullopt); - EXPECT_TRUE(roundtripped->getDiscardableAttr("other_attr") != Attribute()); + EXPECT_EQ(roundtripped->getInherentAttr("inherent_attr"), std::nullopt); + EXPECT_NE(roundtripped->getDiscardableAttr("inherent_attr"), Attribute()); + EXPECT_NE(roundtripped->getDiscardableAttr("other_attr"), Attribute()); EXPECT_TRUE(OperationEquivalence::computeHash(op.get()) == OperationEquivalence::computeHash(roundtripped)); diff --git a/mlir/unittests/IR/OpPropertiesTest.cpp b/mlir/unittests/IR/OpPropertiesTest.cpp index 4759735d9960..bea69e9e8f10 100644 --- a/mlir/unittests/IR/OpPropertiesTest.cpp +++ b/mlir/unittests/IR/OpPropertiesTest.cpp @@ -399,8 +399,9 @@ TEST(OpPropertiesTest, withoutPropertiesDiscardableAttrs) { "other_attr"); EXPECT_EQ(op->getAttrs().size(), 2u); - EXPECT_TRUE(op->getInherentAttr("inherent_attr") != std::nullopt); - EXPECT_TRUE(op->getDiscardableAttr("other_attr") != Attribute()); + EXPECT_EQ(op->getInherentAttr("inherent_attr"), std::nullopt); + EXPECT_NE(op->getDiscardableAttr("inherent_attr"), Attribute()); + EXPECT_NE(op->getDiscardableAttr("other_attr"), Attribute()); std::string output; llvm::raw_string_ostream os(output);