[mlir][IR] Add getPropertyAsAttr and setPropertyFromAttr methods.
#150060
Conversation
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Fabian Mora (fabianmcg) ChangesThis patch adds the The reasoning behind adding these methods is that Also, while it would be possible to use This is also the first step in fixing #150009 . Patch is 24.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/150060.diff 8 Files Affected:
diff --git a/mlir/include/mlir/IR/ExtensibleDialect.h b/mlir/include/mlir/IR/ExtensibleDialect.h
index 955faaad9408b..204b3f59db1a5 100644
--- a/mlir/include/mlir/IR/ExtensibleDialect.h
+++ b/mlir/include/mlir/IR/ExtensibleDialect.h
@@ -493,6 +493,17 @@ class DynamicOpDefinition : public OperationName::Impl {
return failure();
}
Attribute getPropertiesAsAttr(Operation *op) final { return {}; }
+
+ LogicalResult
+ setPropertyFromAttr(OperationName opName, OpaqueProperties properties,
+ StringRef name, Attribute attr,
+ function_ref<InFlightDiagnostic()> emitError) final {
+ emitError() << "extensible Dialects don't support properties";
+ return failure();
+ }
+ FailureOr<Attribute> getPropertyAsAttr(Operation *op, StringRef name) final {
+ return {};
+ }
void copyProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {}
bool compareProperties(OpaqueProperties, OpaqueProperties) final { return false; }
llvm::hash_code hashProperties(OpaqueProperties prop) final { return {}; }
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 883ece32967e4..2e610ec21000c 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -1758,6 +1758,17 @@ class Op : public OpState, public Traits<ConcreteType>... {
function_ref<InFlightDiagnostic()> emitError) {
return setPropertiesFromAttribute(prop, attr, emitError);
}
+ /// Convert the provided attribute to a property and assigned it to the
+ /// corresponding property. This default implementation forwards to a free
+ /// function `setPropertiesFromAttribute` that can be looked up with ADL in
+ /// the namespace where the properties are defined. It can also be overridden
+ /// in the derived ConcreteOp.
+ template <typename PropertiesTy>
+ static LogicalResult
+ setPropertyFromAttr(PropertiesTy &prop, StringRef name, Attribute attr,
+ function_ref<InFlightDiagnostic()> emitError) {
+ return setPropertyFromAttribute(prop, name, attr, emitError);
+ }
/// Convert the provided properties to an attribute. This default
/// implementation forwards to a free function `getPropertiesAsAttribute` that
/// can be looked up with ADL in the namespace where the properties are
@@ -1767,6 +1778,16 @@ class Op : public OpState, public Traits<ConcreteType>... {
const PropertiesTy &prop) {
return getPropertiesAsAttribute(ctx, prop);
}
+ /// Convert the provided named property to an attribute. This default
+ /// implementation forwards to a free function `getPropertiesAsAttribute` that
+ /// can be looked up with ADL in the namespace where the properties are
+ /// defined. It can also be overridden in the derived ConcreteOp.
+ template <typename PropertiesTy>
+ static FailureOr<Attribute> getPropertyAsAttr(MLIRContext *ctx,
+ const PropertiesTy &prop,
+ StringRef name) {
+ return getPropertyAsAttribute(ctx, prop, name);
+ }
/// Hash the provided properties. This default implementation forwards to a
/// free function `computeHash` that can be looked up with ADL in the
/// namespace where the properties are defined. It can also be overridden in
diff --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h
index fa8a4873572ce..42ab61c055bc3 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -920,6 +920,12 @@ class alignas(8) Operation final
/// operation. Returns an empty attribute if no properties are present.
Attribute getPropertiesAsAttribute();
+ /// Return a named property converted to an attribute.
+ /// This is expensive, and mostly useful when dealing with unregistered
+ /// operations or in language bindings. Returns failure if there's no property
+ /// under such name.
+ FailureOr<Attribute> getPropertyAsAttribute(StringRef name);
+
/// Set the properties from the provided attribute.
/// This is an expensive operation that can fail if the attribute is not
/// matching the expectations of the properties for this operation. This is
@@ -930,6 +936,17 @@ class alignas(8) Operation final
setPropertiesFromAttribute(Attribute attr,
function_ref<InFlightDiagnostic()> emitError);
+ /// Set a named property from the provided attribute.
+ /// This is an expensive operation that can fail if the attribute is not
+ /// matching the expectations of the properties for this operation. This is
+ /// mostly useful for unregistered operations, used when parsing the
+ /// generic format, or in language bindings. An optional diagnostic emitter
+ /// can be passed in for richer errors, if none is passed then behavior is
+ /// undefined in error case.
+ LogicalResult
+ setPropertyFromAttribute(StringRef name, Attribute attr,
+ function_ref<InFlightDiagnostic()> emitError);
+
/// Copy properties from an existing other properties object. The two objects
/// must be the same type.
void copyProperties(OpaqueProperties rhs);
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index 1ff7c56ddca38..7aadcca8f1232 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -139,6 +139,12 @@ class OperationName {
setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
function_ref<InFlightDiagnostic()> emitError) = 0;
virtual Attribute getPropertiesAsAttr(Operation *) = 0;
+ virtual LogicalResult
+ setPropertyFromAttr(OperationName, OpaqueProperties, StringRef name,
+ Attribute,
+ function_ref<InFlightDiagnostic()> emitError) = 0;
+ virtual FailureOr<Attribute> getPropertyAsAttr(Operation *,
+ StringRef name) = 0;
virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual bool compareProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual llvm::hash_code hashProperties(OpaqueProperties) = 0;
@@ -220,6 +226,11 @@ class OperationName {
setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
function_ref<InFlightDiagnostic()> emitError) final;
Attribute getPropertiesAsAttr(Operation *) final;
+ LogicalResult
+ setPropertyFromAttr(OperationName, OpaqueProperties, StringRef name,
+ Attribute,
+ function_ref<InFlightDiagnostic()> emitError) final;
+ FailureOr<Attribute> getPropertyAsAttr(Operation *, StringRef name) final;
void copyProperties(OpaqueProperties, OpaqueProperties) final;
bool compareProperties(OpaqueProperties, OpaqueProperties) final;
llvm::hash_code hashProperties(OpaqueProperties) final;
@@ -441,6 +452,20 @@ class OperationName {
emitError);
}
+ /// Return an op property converted to an Attribute.
+ FailureOr<Attribute> getOpPropertyAsAttribute(Operation *op,
+ StringRef name) const {
+ return getImpl()->getPropertyAsAttr(op, name);
+ }
+
+ /// Define an op property from the provided Attribute.
+ LogicalResult setOpPropertyFromAttribute(
+ OperationName opName, OpaqueProperties properties, StringRef name,
+ Attribute attr, function_ref<InFlightDiagnostic()> emitError) const {
+ return getImpl()->setPropertyFromAttr(opName, properties, name, attr,
+ emitError);
+ }
+
void copyOpProperties(OpaqueProperties lhs, OpaqueProperties rhs) const {
return getImpl()->copyProperties(lhs, rhs);
}
@@ -650,6 +675,26 @@ class RegisteredOperationName : public OperationName {
}
return {};
}
+ LogicalResult
+ setPropertyFromAttr(OperationName opName, OpaqueProperties properties,
+ StringRef name, Attribute attr,
+ function_ref<InFlightDiagnostic()> emitError) final {
+ if constexpr (hasProperties) {
+ auto p = properties.as<Properties *>();
+ return ConcreteOp::setPropertyFromAttr(*p, name, attr, emitError);
+ }
+ emitError() << "this operation does not support properties";
+ return failure();
+ }
+ FailureOr<Attribute> getPropertyAsAttr(Operation *op,
+ StringRef name) final {
+ if constexpr (hasProperties) {
+ auto concreteOp = cast<ConcreteOp>(op);
+ return ConcreteOp::getPropertyAsAttr(concreteOp->getContext(),
+ concreteOp.getProperties(), name);
+ }
+ return failure();
+ }
bool compareProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {
if constexpr (hasProperties) {
return *lhs.as<Properties *>() == *rhs.as<Properties *>();
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index 06ec1c85fb4d5..7b49a945c549b 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -901,6 +901,20 @@ Attribute
OperationName::UnregisteredOpModel::getPropertiesAsAttr(Operation *op) {
return *op->getPropertiesStorage().as<Attribute *>();
}
+LogicalResult OperationName::UnregisteredOpModel::setPropertyFromAttr(
+ OperationName opName, OpaqueProperties properties, StringRef name,
+ Attribute attr, function_ref<InFlightDiagnostic()> emitError) {
+ assert(false &&
+ "`setPropertyFromAttr` doesn't work with unregistered operations.");
+ return failure();
+}
+FailureOr<Attribute>
+OperationName::UnregisteredOpModel::getPropertyAsAttr(Operation *op,
+ StringRef name) {
+ assert(false &&
+ "`getPropertyAsAttr` doesn't work with unregistered operations.");
+ return failure();
+}
void OperationName::UnregisteredOpModel::copyProperties(OpaqueProperties lhs,
OpaqueProperties rhs) {
*lhs.as<Attribute *>() = *rhs.as<Attribute *>();
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 8bcfa465e4a22..edd2efedf5a45 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -351,6 +351,12 @@ Attribute Operation::getPropertiesAsAttribute() {
return *getPropertiesStorage().as<Attribute *>();
return info->getOpPropertiesAsAttribute(this);
}
+FailureOr<Attribute> Operation::getPropertyAsAttribute(StringRef name) {
+ std::optional<RegisteredOperationName> info = getRegisteredInfo();
+ assert(info &&
+ "`getPropertyAsAttribute` only works for registered operations.");
+ return info->getOpPropertyAsAttribute(this, name);
+}
LogicalResult Operation::setPropertiesFromAttribute(
Attribute attr, function_ref<InFlightDiagnostic()> emitError) {
std::optional<RegisteredOperationName> info = getRegisteredInfo();
@@ -361,6 +367,15 @@ LogicalResult Operation::setPropertiesFromAttribute(
return info->setOpPropertiesFromAttribute(
this->getName(), this->getPropertiesStorage(), attr, emitError);
}
+LogicalResult Operation::setPropertyFromAttribute(
+ StringRef name, Attribute attr,
+ function_ref<InFlightDiagnostic()> emitError) {
+ std::optional<RegisteredOperationName> info = getRegisteredInfo();
+ assert(info &&
+ "`setPropertyFromAttribute` only works for registered operations.");
+ return info->setOpPropertyFromAttribute(
+ this->getName(), this->getPropertiesStorage(), name, attr, emitError);
+}
void Operation::copyProperties(OpaqueProperties rhs) {
name.copyOpProperties(getPropertiesStorage(), rhs);
diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index f35cfa6826388..31834554cdd43 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -1411,7 +1411,7 @@ void OpEmitter::genPropertiesSupport() {
attrOrProperties.push_back(&emitHelper.getOperandSegmentsSize().value());
if (emitHelper.getResultSegmentsSize())
attrOrProperties.push_back(&emitHelper.getResultSegmentsSize().value());
- auto &setPropMethod =
+ auto &setPropsMethod =
opClass
.addStaticMethod(
"::llvm::LogicalResult", "setPropertiesFromAttr",
@@ -1421,12 +1421,31 @@ void OpEmitter::genPropertiesSupport() {
"::llvm::function_ref<::mlir::InFlightDiagnostic()>",
"emitError"))
->body();
- auto &getPropMethod =
+ auto &getPropsMethod =
opClass
.addStaticMethod("::mlir::Attribute", "getPropertiesAsAttr",
MethodParameter("::mlir::MLIRContext *", "ctx"),
MethodParameter("const Properties &", "prop"))
->body();
+ auto &setPropMethod =
+ opClass
+ .addStaticMethod(
+ "::llvm::LogicalResult", "setPropertyFromAttr",
+ MethodParameter("Properties &", "prop"),
+ MethodParameter("llvm::StringRef", "name"),
+ MethodParameter("::mlir::Attribute", "attr"),
+ MethodParameter(
+ "::llvm::function_ref<::mlir::InFlightDiagnostic()>",
+ "emitError"))
+ ->body();
+ auto &getPropMethod =
+ opClass
+ .addStaticMethod("llvm::FailureOr<::mlir::Attribute>",
+ "getPropertyAsAttr",
+ MethodParameter("::mlir::MLIRContext *", "ctx"),
+ MethodParameter("const Properties &", "prop"),
+ MethodParameter("llvm::StringRef", "name"))
+ ->body();
auto &hashMethod =
opClass
.addStaticMethod("llvm::hash_code", "computePropertiesHash",
@@ -1468,7 +1487,7 @@ void OpEmitter::genPropertiesSupport() {
// Convert the property to the attribute form.
- setPropMethod << R"decl(
+ setPropsMethod << R"decl(
::mlir::DictionaryAttr dict = ::llvm::dyn_cast<::mlir::DictionaryAttr>(attr);
if (!dict) {
emitError() << "expected DictionaryAttr to set properties";
@@ -1480,9 +1499,9 @@ void OpEmitter::genPropertiesSupport() {
::llvm::function_ref<::mlir::InFlightDiagnostic()> emitError) -> ::mlir::LogicalResult {{
{0}
};
- {1};
+ {1}
)decl";
- const char *attrGetNoDefaultFmt = R"decl(;
+ const char *attrGetNoDefaultFmt = R"decl(
if (attr && ::mlir::failed(setFromAttr(prop.{0}, attr, emitError)))
return ::mlir::failure();
)decl";
@@ -1515,22 +1534,33 @@ void OpEmitter::genPropertiesSupport() {
}
fctx.withBuilder(odsBuilder);
- setPropMethod << "{\n"
+ setPropsMethod << "{\n"
+ << formatv(
+ propFromAttrFmt,
+ tgfmt(prop.getConvertFromAttributeCall(),
+ &fctx.addSubst("_attr", propertyAttr)
+ .addSubst("_storage", propertyStorage)
+ .addSubst("_diag", propertyDiag)),
+ getAttr);
+ if (prop.hasStorageTypeValueOverride()) {
+ setPropsMethod << formatv(attrGetDefaultFmt, name,
+ prop.getStorageTypeValueOverride());
+ } else if (prop.hasDefaultValue()) {
+ setPropsMethod << formatv(attrGetDefaultFmt, name,
+ tgfmt(prop.getDefaultValue(), &fctx));
+ } else {
+ setPropsMethod << formatv(attrGetNoDefaultFmt, name);
+ }
+ setPropsMethod << " }\n";
+ setPropMethod << formatv(" if (name == \"{0}\") {{", name)
<< formatv(propFromAttrFmt,
tgfmt(prop.getConvertFromAttributeCall(),
&fctx.addSubst("_attr", propertyAttr)
.addSubst("_storage", propertyStorage)
.addSubst("_diag", propertyDiag)),
- getAttr);
- if (prop.hasStorageTypeValueOverride()) {
- setPropMethod << formatv(attrGetDefaultFmt, name,
- prop.getStorageTypeValueOverride());
- } else if (prop.hasDefaultValue()) {
- setPropMethod << formatv(attrGetDefaultFmt, name,
- tgfmt(prop.getDefaultValue(), &fctx));
- } else {
- setPropMethod << formatv(attrGetNoDefaultFmt, name);
- }
+ "");
+ setPropMethod << formatv(attrGetNoDefaultFmt, name);
+ setPropMethod << " return ::mlir::success();\n";
setPropMethod << " }\n";
} else {
const auto *namedAttr =
@@ -1548,7 +1578,7 @@ void OpEmitter::genPropertiesSupport() {
os << " if (!attr) attr = dict.get(\"result_segment_sizes\");";
}
- setPropMethod << formatv(R"decl(
+ setPropsMethod << formatv(R"decl(
{{
auto &propStorage = prop.{0};
{1}
@@ -1563,16 +1593,38 @@ void OpEmitter::genPropertiesSupport() {
}
}
)decl",
- name, getAttr);
+ name, getAttr);
+ setPropMethod << formatv(R"decl(
+ {{
+ auto &propStorage = prop.{0};
+ if (attr == nullptr) {{
+ propStorage = nullptr;
+ return ::mlir::success();
+ }
+ auto convertedAttr = ::llvm::dyn_cast<std::remove_reference_t<decltype(propStorage)>>(attr);
+ if (convertedAttr) {{
+ propStorage = convertedAttr;
+ return ::mlir::success();
+ } else {{
+ emitError() << "Invalid attribute `{0}` in property conversion: " << attr;
+ return ::mlir::failure();
}
}
- setPropMethod << " return ::mlir::success();\n";
+)decl",
+ name);
+ }
+ }
+ setPropsMethod << " return ::mlir::success();\n";
+ setPropMethod << " return emitError() << \"`\" << name << \"` is not an op "
+ "property\";\n";
// Convert the attribute form to the property.
- getPropMethod << " ::mlir::SmallVector<::mlir::NamedAttribute> attrs;\n"
- << " ::mlir::Builder odsBuilder{ctx};\n";
- const char *propToAttrFmt = R"decl(
+ getPropsMethod << " ::mlir::SmallVector<::mlir::NamedAttribute> attrs;\n"
+ << " ::mlir::Builder odsBuilder{ctx};\n";
+ getPropMethod << " ::mlir::Builder odsBuilder{ctx};\n"
+ << " (void)odsBuilder;\n";
+ const char *propsToAttrFmt = R"decl(
{
const auto &propStorage = prop.{0};
auto attr = [&]() -> ::mlir::Attribute {{
@@ -1580,6 +1632,15 @@ void OpEmitter::genPropertiesSupport() {
}();
attrs.push_back(odsBuilder.getNamedAttr("{0}", attr));
}
+)decl";
+ const char *propToAttrFmt = R"decl(
+ if (name == "{0}") {
+ const auto &propStorage = prop.{0};
+ auto attr = [&]() -> ::mlir::Attribute {{
+ {1}
+ }();
+ return attr;
+ }
)decl";
for (const auto &attrOrProp : attrOrProperties) {
if (const auto *namedProperty =
@@ -1587,6 +1648,11 @@ void OpEmitter::genPropertiesSupport() {
StringRef name = namedProperty->name;
auto &prop = namedProperty->prop;
FmtContext fctx;
+ getPropsMethod << formatv(
+ propsToAttrFmt, name,
+ tgfmt(prop.getConvertToAttributeCall(),
+ &fctx.addSubst("_ctxt", "ctx")
+ .addSubst("_storage", propertyStorage)));
getPropMethod << formatv(
propToAttrFmt, name,
tgfmt(prop.getConvertToAttributeCall(),
@@ -1597,21 +1663,28 @@ void OpEmitter::genPropertiesSupport() {
const auto *namedAttr =
llvm::dyn_cast_if_present<const AttributeMetadata *>(attrOrProp);
StringRef name = namedAttr->attrName;
- getPropMethod << formatv(R"decl(
+ getPropsMethod << formatv(R"decl(
{{
const auto &propStorage = prop.{0};
if (propStorage)
attrs.push_back(odsBuilder.getNamedAttr("{0}",
propStorage));
}
+)decl",
+ name);
+ getPropMethod << formatv(R"decl(
+ if (name == "{0}") {...
[truncated]
|
fabianmcg
force-pushed
the
users/fabian/pr-get-set-property
branch
from
c912836 to
dd612d6
Compare
It's not really that: these only exists because of the generic format printing/parsing actually, as well as default bytecode emission (if I remember correctly). They aren't meant to be used otherwise.
But I wouldn't want to have to go through attributes to manipulate properties, that defeats the whole point of properties! |
Unfortunately they are also used by the C-API to create ops, see https://github.com/llvm/llvm-project/blob/main/mlir/lib/CAPI/IR/IR.cpp#L615 and https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/Operation.cpp#L41 , which is also the main mechanism to create ops in python. But it's true those funcs are almost never used in the codebase and are there for very specific uses.
I agree on the first point, any of these calls shouldn't be used in C++ code. But properties are an inherently C++ construct, thus interacting with them outside C++ is not trivial. Therefore, something like a tablegen solution for auto generating the bindings like in the C or python case is not longer possible, as users at the very least would have to provide code to teach the bindings how to interact with non-trivial props. Further, we would likely have to generate some binding code for each Op concrete class. While, we could accept this is an acceptable tradeoff, we need to realize it's a significant departure from the current model of the C and python bindings, where all custom ops interact opaquely with the Operation class and there are almost no Op specific functions (ie, we don't generate binding code for each One way to not alter the current model, is to treat properties as attributes outside C++, which IMO is an acceptable tradeoff, because if one really wants to enjoy properties or MLIR to the fullest, then use C++, otherwise pay that price. I'm open to suggestions, because there might be another path that I'm missing. |
But... Attributes are just as much C++ construct though, what's special about properties? |
True, and to get good attribute support we currently have to add custom classes https://github.com/llvm/llvm-project/blob/main/mlir/lib/Bindings/Python/DialectGPU.cpp#L47 . However, even in the absence of those custom bindings it's possible to create attributes in python or C by parsing a string. Something, that's not possible with properties as they there's no generic parsePropertyFromString.
To answer more explicitly, in my view props are a C++ storage detail of the IR element Operation, whereas an attribute is an IR element itself. As such, prop storage doesn't have a well defined meaning outside C++, unlike an Attr, Op or Type that do have a meaning. We could change that, but I don't see a large benefit, and see many potential challenges. |
So: properties don't have a mnemnonic and registration for their parsing logic, but in general they can be parsed/printed if you know the class you want to parse/print I think.
What is the meaning of an Attribute? |
Yes, they can be parsed in the context of an op, but there's still no method to do it outside an op. To use a similar example, yes, we can parse attrtype parameters with a
Attrs are a mechanism to specify constant information, the specific semantics are defined by the attr itself [1], and they have a well defined syntax inside the IR. Now, wrt the second question. An [1] https://mlir.llvm.org/docs/LangRef/#attributes |
|
So, just to write out my vision in more detail:
But we'd also have Where I'd use this is mainly is parsing code. There, /// Hypothetical wrapper around existing methods that sets properties (including inherent attributes)
/// to their default values and tells OperationState and Operation::cretae
/// that inherent and discardable state is being kept separate.
Properties& props = odsState.populateDefaultProperties<Properties>(ctx);
parser.parseOptionalAttrDict([&](StringAttr name, Attribute value) {
if (failed(setPropertyFromAttr(name.getValue(), value)) {
odsState.addAttribute(name, value);
}
return success();
});There'd also be similar parsing logic for |
fabianmcg
changed the title
getPropertyFromAttr and setPropertyFromAttr methods.getPropertyAsAttr and setPropertyFromAttr methods.
I believe this is what I wrote?
Again, I acknowledged this, but what's your point? I don't quite connect this to the overall design question here.
I'm confused: I don't see a "meaning" here? If we're talking semantics and look at for example an IntegerAttr and you're saying that the semantics is that it models an integer, then I don't quite see how different it is from a
What is the semantics outside of an operation that an EnumAttr has that an EnumProp does not? |
That answer was mostly to say: yes, there's some printing and parsing for props, but not in a state to do the same trick we do with attrs with no good binding support where we parse them in python as an alternative. The parameter comment was to say, that in the same way it wouldn't make sense to expose the
IMO, the semantics and meaning of an attr are owned by the attr, while for a prop they are owned (let's say jointly) by the op. An attribute is a standalone unit, that can be parsed, printe, stored generically and have interfaces. A prop only makes sense in the context of the op, and it's mostly a C++ impl detail to store it. |
You're not following up on everything I asked: I mentioned the example of the IntegerAttr to rebute this. An attribute is just like a property: it is only storing some data. All of these get semantics only in conjunction with an operation: the semantics is part of the op definition. The distinction you're making is artificial and does not exist in my mental model.
Sure, we've already covered this: it is only a matter of mnemnonic and registration though.
generically? I don't think so: every attribute comes with a custom storage mechanism, no different than properties. It even have the extra constraints that it needs to be "uniquable".
Yes, interfaces is basically the main blocker to avoid using an Attribute in some cases. But I don't quite see how that would help this PR. |
I'm going to disagree with that one. Attributes and non-attribute properties are both means of storing data of same type T. The semantics of the attribute/property are determined by what data type is being stored there - ops have nothing to do with it. The main distinction is that non-attribute properties lean on the C++ type system directly for how they're stored, while attributes have this whole mechanism where they gen uniqued into the context. The other useful point is that there is an attribute hierarchy (everything is a subclass of The lack of that superclass is why all properties have to be convertible to an Attribute - if you need to work with all the fields in an arbitrary Properties struct, all you can really do is to convert said struct to a collection of name/attribute pairs. Side note re parsing: one could (maybe should) gather up the |
|
Preface, we all have mental models of what props should be and I think we loosely agree on that model. I think where we are diverging is in the mental model of what properties currently are with their def, and I'm being extremely pedantic on this point. That's why I keep insisting, that one option is changing and documenting the model -even if it's not implemented it should be documented.
I didn't mention IntegerProp vs IntgerAttr because I was making my case for *Prop and *Attr. To me they are currently different in all cases.
IMO it's not artificial, it's their current state.
Adding a mnemonic and registration is a big departure from the current model. It would be turning them into "mutable non unique attributes". If that's the path we choose, that's fine, but it must be an explicit decision.
To me this is one the critical distinctions between them, props are just the C++ storage mechanism, similar to the storage class of an attr. Props are missing the ' |
|
To not keep litigating semantics without progressing, how about if we agree on what they should be:
That would mean that in the future after implementing all the missing features:
|
I think that's explicitly an anti-goal. Keeping convertability enables generic IR parsing/printing and certain forms of generic handling / forall (bit of data inherent in an op)-ing That convertability + the more efficient parsing logic is why I was wanting to move to |
And what I'm saying is that for props to become full first-class elements of the IR and not be only a storage C++ impl detail, then conversion through attributes must never be necessary, this includes printing and parsing through generic form, binding interactions, etc... That's what I'm proposing the end goal should be (we can disagree on the end goal). |
I disagree. Conversion to/from attributes should be minimized (ex. non-generic parser/printer could shouldn't need to convert to attributes to handle |
This is the current state. Right now, I'm talking about designing and agreeing on the end model. How many things we need to change to get there is a different issue, and one that I'm saying let's put aside until we agree on the end model. For example, if props are given mnemonics, then something like |
|
Also worth asking as part of this or future conversation, is it still worth it to support generic format and unregistered ops? @ftynse at this point I might take you on the idea of an ODM to quickly clarify all this and move on. Edit: It's clear that for removing generic format we would need to come up how to print non-verifiable ops, which is a biggie. |
Generic format is useful for cases where the pninting logic relies on the verifier. Also, an unregistered operation has a fairly clean property storage, which is almost what's implemented. The properties storage is a string => Attribute dictionary, getPropertyAsAttr() / setPropertyAsAttr() become operations on that dictionary (which can accept arbitrary keys). (The current implementation basically does this, except that it uses a |
|
I'm fundamentally fine with a duplicated structure between attributes and non-attribute fields of a I don't understand what end model you're going for here. I think the current system is quite workable, with the caveat that a bunch of infrastructure is missing (ex., parsers should be doing the switch-case into the properties struct, Python bindings, etc.) and that we've got a whole bunch of stuff leaning on getAttrDictionary() and other APIs that aren't making a careful inherent/discardable distinction. |
Side note, but IMO that's not an argument for using an attribute: attributes are leaky, so unsuitable for large elements. |
This patch adds the
getPropertyAsAttrandsetPropertyFromAttrmethods to various places. These methods are similar togetPropertiesAsAttrandsetPropertiesFromAttr, in that they get or set a property to or from an attr, but they set or get props based on a name.The reasoning behind adding these methods is that
getPropertiesAsAttrandsetPropertiesFromAttrare meant for getting and setting collections of props, which is not always desirable specially in language bindings like the python bindings, and can be wasteful.Also, while it would be possible to use
setPropertiesFromAttrto set a single prop, the method introduces additional logic to set default values if the value is not present, which is not something desirable once the op has been created. Further,getPropertiesAsAttralways converts all properties, which might be a wasteful process for large props.This is also the first step in fixing #150009 .