[mlir][AsmPrinter] Print op properties directly in generic form #106996
Conversation
As properties are always at least default-constructed (during Operation's creation), they should be printable at any point. The AsmPrinter's code is overly cautious and always selects to print properties as attributes in the "generic op form". Instead, this decision could be moved to the operation. There's already some infrastructure around printing (with extension points available) that by default converts the properties to attributes (so the current behaviour is majorly unchaged). This way, users not wishing to use "convert to property" fallback could provide a custom printing method that is going to be used during "generic op form" printing (e.g. when op verification fails). The assumption is that if we could convert properties to attributes in "failure" state, it should also be possible to print the properties directly.
|
@joker-eph I think you would be the best person to review this. |
|
@llvm/pr-subscribers-mlir-gpu @llvm/pr-subscribers-mlir-core Author: Andrei Golubev (andrey-golubev) ChangesAs properties are always at least default-constructed (during Operation's creation), they should be printable at any point. The AsmPrinter's code is overly cautious and always selects to print properties as attributes in the "generic op form". Instead, this decision could be moved to the operation. There's already some infrastructure around printing (with extension points available) that by default converts the properties to attributes (so the current behaviour is majorly unchaged). This way, users not wishing to use "convert to property" fallback could provide a custom printing method that is going to be used during "generic op form" printing (e.g. when op verification fails). The assumption is that if we could convert properties to attributes in "failure" state, it should also be possible to print the properties directly. Full diff: https://github.com/llvm/llvm-project/pull/106996.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td b/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td
index c32c7541c39791..c88a976240bb26 100644
--- a/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td
+++ b/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td
@@ -31,7 +31,7 @@ class XeGPU_Op<string mnemonic, list<Trait> traits = []>:
::mlir::ArrayRef<::llvm::StringRef> elidedProps) {
Attribute propAttr = getPropertiesAsAttr(ctx, prop);
if (propAttr)
- p << "<" << propAttr << ">";
+ p << " <" << propAttr << ">";
}
static ::mlir::ParseResult parseProperties(::mlir::OpAsmParser &parser,
diff --git a/mlir/include/mlir/IR/ExtensibleDialect.h b/mlir/include/mlir/IR/ExtensibleDialect.h
index 494f3dfb05a04d..ffb9fa9ef467a2 100644
--- a/mlir/include/mlir/IR/ExtensibleDialect.h
+++ b/mlir/include/mlir/IR/ExtensibleDialect.h
@@ -496,6 +496,7 @@ class DynamicOpDefinition : public OperationName::Impl {
void copyProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {}
bool compareProperties(OpaqueProperties, OpaqueProperties) final { return false; }
llvm::hash_code hashProperties(OpaqueProperties prop) final { return {}; }
+ void printProperties(Operation *, OpAsmPrinter &) final {}
private:
DynamicOpDefinition(
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 59f094d6690991..b8788b22080150 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -1809,14 +1809,13 @@ class Op : public OpState, public Traits<ConcreteType>... {
/// Trait to check if printProperties(OpAsmPrinter, T, ArrayRef<StringRef>)
/// exist
- template <typename T, typename... Args>
- using has_print_properties =
- decltype(printProperties(std::declval<OpAsmPrinter &>(),
- std::declval<T>(),
- std::declval<ArrayRef<StringRef>>()));
- template <typename T>
+ template <typename ConcreteOp, typename Props>
+ using has_print_properties = decltype(ConcreteOp::printProperties(
+ std::declval<OpAsmPrinter &>(), std::declval<Props>(),
+ std::declval<ArrayRef<StringRef>>()));
+ template <typename ConcreteOp, typename Props>
using detect_has_print_properties =
- llvm::is_detected<has_print_properties, T>;
+ llvm::is_detected<has_print_properties, ConcreteOp, Props>;
/// Trait to check if parseProperties(OpAsmParser, T) exist
template <typename T, typename... Args>
@@ -1981,16 +1980,16 @@ class Op : public OpState, public Traits<ConcreteType>... {
/// Print the operation properties with names not included within
/// 'elidedProps'. Unless overridden, this method will try to dispatch to a
- /// `printProperties` free-function if it exists, and otherwise by converting
- /// the properties to an Attribute.
- template <typename T>
+ /// `T::printProperties` if it exists, and otherwise by converting the
+ /// properties to an Attribute.
+ template <typename T = ConcreteType>
static void printProperties(MLIRContext *ctx, OpAsmPrinter &p,
- const T &properties,
+ const InferredProperties<T> &properties,
ArrayRef<StringRef> elidedProps = {}) {
- if constexpr (detect_has_print_properties<T>::value)
- return printProperties(p, properties, elidedProps);
- genericPrintProperties(
- p, ConcreteType::getPropertiesAsAttr(ctx, properties), elidedProps);
+ if constexpr (detect_has_print_properties<T, InferredProperties<T>>::value)
+ return T::printProperties(p, properties, elidedProps);
+ genericPrintProperties(p, T::getPropertiesAsAttr(ctx, properties),
+ elidedProps);
}
/// Parses 'prop-dict' for the operation. Unless overridden, the method will
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index 1b93f3d3d04fe8..51ec61ea15b3c4 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -142,6 +142,7 @@ class OperationName {
virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual bool compareProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual llvm::hash_code hashProperties(OpaqueProperties) = 0;
+ virtual void printProperties(Operation *, OpAsmPrinter &) = 0;
};
public:
@@ -223,6 +224,7 @@ class OperationName {
void copyProperties(OpaqueProperties, OpaqueProperties) final;
bool compareProperties(OpaqueProperties, OpaqueProperties) final;
llvm::hash_code hashProperties(OpaqueProperties) final;
+ void printProperties(Operation *, OpAsmPrinter &) final;
};
public:
@@ -455,6 +457,10 @@ class OperationName {
return getImpl()->hashProperties(properties);
}
+ void printOpProperties(Operation *op, OpAsmPrinter &printer) const {
+ getImpl()->printProperties(op, printer);
+ }
+
/// Return the dialect this operation is registered to if the dialect is
/// loaded in the context, or nullptr if the dialect isn't loaded.
Dialect *getDialect() const {
@@ -668,6 +674,13 @@ class RegisteredOperationName : public OperationName {
return {};
}
+ void printProperties(Operation *op, OpAsmPrinter &printer) final {
+ if constexpr (hasProperties) {
+ auto concreteOp = cast<ConcreteOp>(op);
+ ConcreteOp::printProperties(concreteOp->getContext(), printer,
+ concreteOp.getProperties());
+ }
+ }
};
/// Lookup the registered operation information for the given operation.
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index 02acc8c3f4659e..0db6bccf4cfeaf 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -3558,10 +3558,8 @@ void OperationPrinter::printGenericOp(Operation *op, bool printOpName) {
}
// Print the properties.
- if (Attribute prop = op->getPropertiesAsAttribute()) {
- os << " <";
- Impl::printAttribute(prop);
- os << '>';
+ if (op->getPropertiesStorageSize()) {
+ op->getName().printOpProperties(op, *this);
}
// Print regions.
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index 5c93747438ecdb..cbf544e34a9f0a 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -908,6 +908,16 @@ llvm::hash_code
OperationName::UnregisteredOpModel::hashProperties(OpaqueProperties prop) {
return llvm::hash_combine(*prop.as<Attribute *>());
}
+void OperationName::UnregisteredOpModel::printProperties(
+ Operation *op, OpAsmPrinter &printer) {
+ auto asAttr = getPropertiesAsAttr(op);
+ // Note: printAttribute(nullptr) would insert <<NULL ATTRIBUTE>> which changes
+ // the current behavior
+ if (asAttr == nullptr)
+ return;
+ printer << ' ';
+ printer.printAttribute(asAttr);
+}
//===----------------------------------------------------------------------===//
// RegisteredOperationName
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 8f28d61c732cdc..e2f393ec3d207c 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -803,6 +803,7 @@ void OpState::genericPrintProperties(OpAsmPrinter &p, Attribute properties,
ArrayRef<StringRef> elidedProps) {
if (!properties)
return;
+ p << ' ';
auto dictAttr = dyn_cast_or_null<::mlir::DictionaryAttr>(properties);
if (dictAttr && !elidedProps.empty()) {
ArrayRef<NamedAttribute> attrs = dictAttr.getValue();
diff --git a/mlir/test/IR/properties.mlir b/mlir/test/IR/properties.mlir
index 418b81dcbb034f..5d64aa8b6024da 100644
--- a/mlir/test/IR/properties.mlir
+++ b/mlir/test/IR/properties.mlir
@@ -79,3 +79,8 @@ test.with_optional_properties nested = some<none>
// CHECK-SAME: ints = [1, 2] strings = ["a", "b"] nested = {{\[}}[1, 2], [3, 4]] opt = [-1, -2] explicitOptions = [none, 0] explicitUnits = [unit, unit_absent]
// GENERIC: "test.with_array_properties"()
test.with_array_properties ints = [1, 2] strings = ["a", "b"] nested = [[1, 2], [3, 4]] opt = [-1, -2] explicitOptions = [none, 0] explicitUnits = [unit, unit_absent] [] thats_has_default
+
+// CHECK: test.with_print_properties_hook a = 42{{$}}
+// GENERIC: "test.with_print_properties_hook"()
+// GENERIC-SAME: <{printing_through_custom_hook = true, a = 42}> : () -> ()
+test.with_print_properties_hook a = 42
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index 9e19966414d1d7..f146564b90a288 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -3150,6 +3150,27 @@ def TestOpWithVersionedProperties : TEST_Op<"with_versioned_properties"> {
}];
}
+def TestOpWithPrintPropertiesHook : TEST_Op<"with_print_properties_hook"> {
+ let assemblyFormat = "`a` `=` $a attr-dict";
+ let arguments = (ins
+ I64Property:$a
+ );
+ let extraClassDeclaration = [{
+ static void printProperties(::mlir::OpAsmPrinter &p,
+ const Properties &prop,
+ ::mlir::ArrayRef<::llvm::StringRef>);
+ }];
+ let extraClassDefinition = [{
+ void TestOpWithPrintPropertiesHook::printProperties(
+ ::mlir::OpAsmPrinter &p, const Properties &prop,
+ ::mlir::ArrayRef<::llvm::StringRef>) {
+ // Note: we need to comply with MLIR's asm parser, so "pretend" we're
+ // printing an attribute sequence
+ p << "<{printing_through_custom_hook = true, a = " << prop.a << "}>";
+ }
+ }];
+}
+
def TestOpWithDefaultValuedProperties : TEST_Op<"with_default_valued_properties"> {
let assemblyFormat = [{
($a^) : (`na`)?
diff --git a/mlir/tools/mlir-tblgen/OpClass.cpp b/mlir/tools/mlir-tblgen/OpClass.cpp
index 60fa1833ce625e..c3f5d5f1bf486f 100644
--- a/mlir/tools/mlir-tblgen/OpClass.cpp
+++ b/mlir/tools/mlir-tblgen/OpClass.cpp
@@ -26,6 +26,7 @@ OpClass::OpClass(StringRef name, std::string extraClassDeclaration,
/// Inherit functions from Op.
declare<UsingDeclaration>("Op::Op");
declare<UsingDeclaration>("Op::print");
+ declare<UsingDeclaration>("Op::printProperties");
/// Type alias for the adaptor class.
declare<UsingDeclaration>("Adaptor", className + "Adaptor");
declare<UsingDeclaration>("GenericAdaptor",
|
|
@llvm/pr-subscribers-mlir Author: Andrei Golubev (andrey-golubev) ChangesAs properties are always at least default-constructed (during Operation's creation), they should be printable at any point. The AsmPrinter's code is overly cautious and always selects to print properties as attributes in the "generic op form". Instead, this decision could be moved to the operation. There's already some infrastructure around printing (with extension points available) that by default converts the properties to attributes (so the current behaviour is majorly unchaged). This way, users not wishing to use "convert to property" fallback could provide a custom printing method that is going to be used during "generic op form" printing (e.g. when op verification fails). The assumption is that if we could convert properties to attributes in "failure" state, it should also be possible to print the properties directly. Full diff: https://github.com/llvm/llvm-project/pull/106996.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td b/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td
index c32c7541c39791..c88a976240bb26 100644
--- a/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td
+++ b/mlir/include/mlir/Dialect/XeGPU/IR/XeGPUOps.td
@@ -31,7 +31,7 @@ class XeGPU_Op<string mnemonic, list<Trait> traits = []>:
::mlir::ArrayRef<::llvm::StringRef> elidedProps) {
Attribute propAttr = getPropertiesAsAttr(ctx, prop);
if (propAttr)
- p << "<" << propAttr << ">";
+ p << " <" << propAttr << ">";
}
static ::mlir::ParseResult parseProperties(::mlir::OpAsmParser &parser,
diff --git a/mlir/include/mlir/IR/ExtensibleDialect.h b/mlir/include/mlir/IR/ExtensibleDialect.h
index 494f3dfb05a04d..ffb9fa9ef467a2 100644
--- a/mlir/include/mlir/IR/ExtensibleDialect.h
+++ b/mlir/include/mlir/IR/ExtensibleDialect.h
@@ -496,6 +496,7 @@ class DynamicOpDefinition : public OperationName::Impl {
void copyProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {}
bool compareProperties(OpaqueProperties, OpaqueProperties) final { return false; }
llvm::hash_code hashProperties(OpaqueProperties prop) final { return {}; }
+ void printProperties(Operation *, OpAsmPrinter &) final {}
private:
DynamicOpDefinition(
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 59f094d6690991..b8788b22080150 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -1809,14 +1809,13 @@ class Op : public OpState, public Traits<ConcreteType>... {
/// Trait to check if printProperties(OpAsmPrinter, T, ArrayRef<StringRef>)
/// exist
- template <typename T, typename... Args>
- using has_print_properties =
- decltype(printProperties(std::declval<OpAsmPrinter &>(),
- std::declval<T>(),
- std::declval<ArrayRef<StringRef>>()));
- template <typename T>
+ template <typename ConcreteOp, typename Props>
+ using has_print_properties = decltype(ConcreteOp::printProperties(
+ std::declval<OpAsmPrinter &>(), std::declval<Props>(),
+ std::declval<ArrayRef<StringRef>>()));
+ template <typename ConcreteOp, typename Props>
using detect_has_print_properties =
- llvm::is_detected<has_print_properties, T>;
+ llvm::is_detected<has_print_properties, ConcreteOp, Props>;
/// Trait to check if parseProperties(OpAsmParser, T) exist
template <typename T, typename... Args>
@@ -1981,16 +1980,16 @@ class Op : public OpState, public Traits<ConcreteType>... {
/// Print the operation properties with names not included within
/// 'elidedProps'. Unless overridden, this method will try to dispatch to a
- /// `printProperties` free-function if it exists, and otherwise by converting
- /// the properties to an Attribute.
- template <typename T>
+ /// `T::printProperties` if it exists, and otherwise by converting the
+ /// properties to an Attribute.
+ template <typename T = ConcreteType>
static void printProperties(MLIRContext *ctx, OpAsmPrinter &p,
- const T &properties,
+ const InferredProperties<T> &properties,
ArrayRef<StringRef> elidedProps = {}) {
- if constexpr (detect_has_print_properties<T>::value)
- return printProperties(p, properties, elidedProps);
- genericPrintProperties(
- p, ConcreteType::getPropertiesAsAttr(ctx, properties), elidedProps);
+ if constexpr (detect_has_print_properties<T, InferredProperties<T>>::value)
+ return T::printProperties(p, properties, elidedProps);
+ genericPrintProperties(p, T::getPropertiesAsAttr(ctx, properties),
+ elidedProps);
}
/// Parses 'prop-dict' for the operation. Unless overridden, the method will
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index 1b93f3d3d04fe8..51ec61ea15b3c4 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -142,6 +142,7 @@ class OperationName {
virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual bool compareProperties(OpaqueProperties, OpaqueProperties) = 0;
virtual llvm::hash_code hashProperties(OpaqueProperties) = 0;
+ virtual void printProperties(Operation *, OpAsmPrinter &) = 0;
};
public:
@@ -223,6 +224,7 @@ class OperationName {
void copyProperties(OpaqueProperties, OpaqueProperties) final;
bool compareProperties(OpaqueProperties, OpaqueProperties) final;
llvm::hash_code hashProperties(OpaqueProperties) final;
+ void printProperties(Operation *, OpAsmPrinter &) final;
};
public:
@@ -455,6 +457,10 @@ class OperationName {
return getImpl()->hashProperties(properties);
}
+ void printOpProperties(Operation *op, OpAsmPrinter &printer) const {
+ getImpl()->printProperties(op, printer);
+ }
+
/// Return the dialect this operation is registered to if the dialect is
/// loaded in the context, or nullptr if the dialect isn't loaded.
Dialect *getDialect() const {
@@ -668,6 +674,13 @@ class RegisteredOperationName : public OperationName {
return {};
}
+ void printProperties(Operation *op, OpAsmPrinter &printer) final {
+ if constexpr (hasProperties) {
+ auto concreteOp = cast<ConcreteOp>(op);
+ ConcreteOp::printProperties(concreteOp->getContext(), printer,
+ concreteOp.getProperties());
+ }
+ }
};
/// Lookup the registered operation information for the given operation.
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index 02acc8c3f4659e..0db6bccf4cfeaf 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -3558,10 +3558,8 @@ void OperationPrinter::printGenericOp(Operation *op, bool printOpName) {
}
// Print the properties.
- if (Attribute prop = op->getPropertiesAsAttribute()) {
- os << " <";
- Impl::printAttribute(prop);
- os << '>';
+ if (op->getPropertiesStorageSize()) {
+ op->getName().printOpProperties(op, *this);
}
// Print regions.
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index 5c93747438ecdb..cbf544e34a9f0a 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -908,6 +908,16 @@ llvm::hash_code
OperationName::UnregisteredOpModel::hashProperties(OpaqueProperties prop) {
return llvm::hash_combine(*prop.as<Attribute *>());
}
+void OperationName::UnregisteredOpModel::printProperties(
+ Operation *op, OpAsmPrinter &printer) {
+ auto asAttr = getPropertiesAsAttr(op);
+ // Note: printAttribute(nullptr) would insert <<NULL ATTRIBUTE>> which changes
+ // the current behavior
+ if (asAttr == nullptr)
+ return;
+ printer << ' ';
+ printer.printAttribute(asAttr);
+}
//===----------------------------------------------------------------------===//
// RegisteredOperationName
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 8f28d61c732cdc..e2f393ec3d207c 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -803,6 +803,7 @@ void OpState::genericPrintProperties(OpAsmPrinter &p, Attribute properties,
ArrayRef<StringRef> elidedProps) {
if (!properties)
return;
+ p << ' ';
auto dictAttr = dyn_cast_or_null<::mlir::DictionaryAttr>(properties);
if (dictAttr && !elidedProps.empty()) {
ArrayRef<NamedAttribute> attrs = dictAttr.getValue();
diff --git a/mlir/test/IR/properties.mlir b/mlir/test/IR/properties.mlir
index 418b81dcbb034f..5d64aa8b6024da 100644
--- a/mlir/test/IR/properties.mlir
+++ b/mlir/test/IR/properties.mlir
@@ -79,3 +79,8 @@ test.with_optional_properties nested = some<none>
// CHECK-SAME: ints = [1, 2] strings = ["a", "b"] nested = {{\[}}[1, 2], [3, 4]] opt = [-1, -2] explicitOptions = [none, 0] explicitUnits = [unit, unit_absent]
// GENERIC: "test.with_array_properties"()
test.with_array_properties ints = [1, 2] strings = ["a", "b"] nested = [[1, 2], [3, 4]] opt = [-1, -2] explicitOptions = [none, 0] explicitUnits = [unit, unit_absent] [] thats_has_default
+
+// CHECK: test.with_print_properties_hook a = 42{{$}}
+// GENERIC: "test.with_print_properties_hook"()
+// GENERIC-SAME: <{printing_through_custom_hook = true, a = 42}> : () -> ()
+test.with_print_properties_hook a = 42
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index 9e19966414d1d7..f146564b90a288 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -3150,6 +3150,27 @@ def TestOpWithVersionedProperties : TEST_Op<"with_versioned_properties"> {
}];
}
+def TestOpWithPrintPropertiesHook : TEST_Op<"with_print_properties_hook"> {
+ let assemblyFormat = "`a` `=` $a attr-dict";
+ let arguments = (ins
+ I64Property:$a
+ );
+ let extraClassDeclaration = [{
+ static void printProperties(::mlir::OpAsmPrinter &p,
+ const Properties &prop,
+ ::mlir::ArrayRef<::llvm::StringRef>);
+ }];
+ let extraClassDefinition = [{
+ void TestOpWithPrintPropertiesHook::printProperties(
+ ::mlir::OpAsmPrinter &p, const Properties &prop,
+ ::mlir::ArrayRef<::llvm::StringRef>) {
+ // Note: we need to comply with MLIR's asm parser, so "pretend" we're
+ // printing an attribute sequence
+ p << "<{printing_through_custom_hook = true, a = " << prop.a << "}>";
+ }
+ }];
+}
+
def TestOpWithDefaultValuedProperties : TEST_Op<"with_default_valued_properties"> {
let assemblyFormat = [{
($a^) : (`na`)?
diff --git a/mlir/tools/mlir-tblgen/OpClass.cpp b/mlir/tools/mlir-tblgen/OpClass.cpp
index 60fa1833ce625e..c3f5d5f1bf486f 100644
--- a/mlir/tools/mlir-tblgen/OpClass.cpp
+++ b/mlir/tools/mlir-tblgen/OpClass.cpp
@@ -26,6 +26,7 @@ OpClass::OpClass(StringRef name, std::string extraClassDeclaration,
/// Inherit functions from Op.
declare<UsingDeclaration>("Op::Op");
declare<UsingDeclaration>("Op::print");
+ declare<UsingDeclaration>("Op::printProperties");
/// Type alias for the adaptor class.
declare<UsingDeclaration>("Adaptor", className + "Adaptor");
declare<UsingDeclaration>("GenericAdaptor",
|
| if constexpr (hasProperties) { | ||
| auto concreteOp = cast<ConcreteOp>(op); | ||
| ConcreteOp::printProperties(concreteOp->getContext(), printer, | ||
| concreteOp.getProperties()); |
There was a problem hiding this comment.
self-review: perhaps I should be explicit here to also pass ArrayRef<StringRef>{} as the 4th argument?
(e.g. see XeGPU_Op's declaration of this method)
| @@ -1809,14 +1809,13 @@ class Op : public OpState, public Traits<ConcreteType>... { | |||
|
|
|||
| /// Trait to check if printProperties(OpAsmPrinter, T, ArrayRef<StringRef>) | |||
There was a problem hiding this comment.
self-review:
| /// Trait to check if printProperties(OpAsmPrinter, T, ArrayRef<StringRef>) | |
| /// Trait to check if ConcreteType::printProperties(OpAsmPrinter, T, ArrayRef<StringRef>) |
| std::declval<ArrayRef<StringRef>>())); | ||
| template <typename T> | ||
| template <typename ConcreteOp, typename Props> | ||
| using has_print_properties = decltype(ConcreteOp::printProperties( |
There was a problem hiding this comment.
note: this changed from free-standing printProperties to ConcreteOp::printProperties. motivation:
I've tried to have a custom "hook" using a free-standing function and failed (I think because the declaration order was messed up). At which point I figured that the last thing the user wants is a no-error-whatsoever (thanks SFINAE) mechanism to supply a custom callback.
Since properties type is strongly coupled to the operation, i think it makes sense to make printProperties a static member function also. This would at least avoid the declaration-issue as one would have method declaration in the body of the Op (e.g. when generating tablegen - inside extraClassDeclarations).
|
The infra is setup so that you can parse back an IR in the generic form without registering the operation parser: you would introduce some sort of a custom syntax here in the generic form that can't be generically parsed. Seems like a departure of the generic format invariant? |
Oh, you're right, I think I missed that. Would it be an option to also have "parseProperties" (if it's not there already) in the generic op form? Fwiw, the real problem I'm trying to solve is "do not convert properties to attributes". Apparently, in my case I could get a generic-op-form printer to fire sometimes (e.g. from logs) and this would go through a conversion (that I meticulously try to abolish). |
|
The sole purpose of why the convert-to-attribute path was included in the development of properties was for the purpose of the generic printer (IIRC). So I am not sure what it would mean to add an escape hatch here really: if we were to accept this then the convert-to-attribute may just go away instead. |
What is the reasoning behind this and could this be solved some other way maybe? |
I think this could be a good way out, actually. Let me look at it a bit more, perhaps i'll see how to make it work. I'd ideally want to avoid large refactorings, though, which is why I went with "OK, I can do a hook here and all is good" approach :) (most of the infra - even the hook - was already present even).
Do you mean why it happens or why I want to avoid this?
|
Why is « default constructed » important here? Also, back to the invariant of the generic format: « we can parse it back without registering the op » has been what we preserved so far. I am a bit nervous about breaking this. |
Yes, I completely agree here. We should not under any circumstance break the ability to parse generic IR when dialects aren't registered. One thing that has been in mind though, why not change properties to just preserve the string form the same way we do for UnknownAttr/UnknownType when the op isn't registered? I don't think there is a strict requirement that we must parse/print as attributes in the generic form, and preserving the string in the unknown case would allow for more optimal roundtripability (I would love to kill the property<->attribute conversion stuff). |
as an aside, We are not totally enforcing this: Attribute And types require their dialect to be registered already. The string idea could work I think! |
Never mind, just me explaining the thing poorly. What i meant is: at the point of printing, properties are already constructed one way or another, so printing them "unconditionally" should be possible. For a property of type
Could you elaborate? I think I'm out of context here. Is the main problem in parsing? I.e. if we end up having unregistered operation, we go through UnregisteredOpModel and that one has to store properties in some form (now, as a dict attr I guess) until we load all the necessary info and are able to "restore" concrete C++ objects (aka properties)? If you could point me to the places of interest in code, it would be nice, maybe that way I'd have a better idea (or maybe there are tests). |
Yeah that is the concern: right now the fact that we emit an attribute means we can always parse it back without the op parser registered. |
Yes, the problem is parsing things back in when you don't have anything registered. When we print, we shouldn't technically need to convert to an attribute (that's just how it works right now). For Types and Attributes, we always print them using the registered hooks. When parsing, if the dialect isn't registered we create UnknownAttr or UnknownType, which effectively just hold the dialect name and the raw string that we parsed from the source .mlir file. Take a look at the parser code for Attributes and Types. Handling things this way enables for roundtripping attributes/types in unregistered dialects as a "pass through" effectively. For properties I would hope we could do the same thing: When parsing a property dictionary for an unregistered operation, store the full parsed string. When printing we just splat it back out (so from a textual perspective it generates the same thing exactly). |
As properties are always at least default-constructed (during Operation's creation), they should be printable at any point.
The AsmPrinter's code is overly cautious and always selects to print properties as attributes in the "generic op form". Instead, this decision could be moved to the operation. There's already some infrastructure around printing (with extension points available) that by default converts the properties to attributes (so the current behaviour is majorly unchaged).
This way, users not wishing to use "convert to property" fallback could provide a custom printing method that is going to be used during "generic op form" printing (e.g. when op verification fails). The assumption is that if we could convert properties to attributes in "failure" state, it should also be possible to print the properties directly.