[Mlir-commits] [mlir] [mlir] Refactor opaque properties to make them type-safe (PR #185157)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Sat Mar 14 23:14:00 PDT 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-core
Author: Krzysztof Drewniak (krzysz00)
<details>
<summary>Changes</summary>
At its core, this commit changes `OpaqueProperties` (aka a void*) to `PropertyRef`, which is a {TypeID, void*}, where the TypeID is the ID of the storage type of the given property (which can, as is often the case for operations, be a struct of other properties).
Long-term, this change will allow for
1) Some sort of getFooPropertyRef() on property structs, allowing individual members to be extracted generically
2) By having a property kind that is an OwningProprtyRef, generic parsing (in combination with a bunch of other changes) 3) Probably a safer C/Python API because we'll be able to indicate what's supposed to be under a given void*
While I was here, Claude noticed that the dynamic dialect code was such that no two operations would ever be deemed equivalent because their (zero-sized, nonexistent) properties would compare false. That's been fixed.
---
Patch is 69.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/185157.diff
30 Files Affected:
- (modified) mlir/include/mlir/IR/ExtensibleDialect.h (+8-10)
- (modified) mlir/include/mlir/IR/OpDefinition.h (+7-4)
- (modified) mlir/include/mlir/IR/Operation.h (+24-21)
- (modified) mlir/include/mlir/IR/OperationSupport.h (+84-65)
- (modified) mlir/include/mlir/Interfaces/InferTypeOpInterface.td (+7-7)
- (modified) mlir/lib/CAPI/IR/IR.cpp (+3-3)
- (modified) mlir/lib/CAPI/Interfaces/Interfaces.cpp (+13-2)
- (modified) mlir/lib/Dialect/AMDGPU/IR/AMDGPUOps.cpp (+1-1)
- (modified) mlir/lib/Dialect/Bufferization/IR/BufferizationOps.cpp (+1-1)
- (modified) mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp (+1-1)
- (modified) mlir/lib/Dialect/Ptr/IR/PtrDialect.cpp (+1-1)
- (modified) mlir/lib/Dialect/SMT/IR/SMTOps.cpp (+2-2)
- (modified) mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp (+6-8)
- (modified) mlir/lib/Dialect/Tosa/IR/TosaOps.cpp (+5-5)
- (modified) mlir/lib/Dialect/WasmSSA/IR/WasmSSAOps.cpp (+3-3)
- (modified) mlir/lib/IR/ExtensibleDialect.cpp (+2)
- (modified) mlir/lib/IR/MLIRContext.cpp (+11-11)
- (modified) mlir/lib/IR/Operation.cpp (+9-12)
- (modified) mlir/lib/IR/OperationSupport.cpp (+2)
- (modified) mlir/lib/Target/IRDLToCpp/Templates/PerOperationDecl.txt (+1-1)
- (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+4-4)
- (modified) mlir/test/lib/Dialect/Test/TestOpDefs.cpp (+6-6)
- (modified) mlir/test/lib/Dialect/Test/TestOps.td (+2-2)
- (modified) mlir/test/lib/Dialect/Test/TestOpsSyntax.cpp (+1-1)
- (modified) mlir/test/lib/Dialect/Test/TestOpsSyntax.td (+4-4)
- (modified) mlir/test/lib/Dialect/Test/TestPatterns.cpp (+5-2)
- (modified) mlir/test/mlir-tblgen/op-decl-and-defs.td (+1-1)
- (modified) mlir/test/python/python_test_ops.td (+3-3)
- (modified) mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp (+28-15)
- (modified) mlir/unittests/Debug/FileLineColLocBreakpointManagerTest.cpp (+1-1)
``````````diff
diff --git a/mlir/include/mlir/IR/ExtensibleDialect.h b/mlir/include/mlir/IR/ExtensibleDialect.h
index dcbf6813506d5..f6b82bffcfffc 100644
--- a/mlir/include/mlir/IR/ExtensibleDialect.h
+++ b/mlir/include/mlir/IR/ExtensibleDialect.h
@@ -550,25 +550,23 @@ class DynamicOpDefinition : public OperationName::Impl {
return success();
}
int getOpPropertyByteSize() final { return 0; }
- void initProperties(OperationName opName, OpaqueProperties storage,
- OpaqueProperties init) final {}
- void deleteProperties(OpaqueProperties prop) final {}
+ void initProperties(OperationName opName, PropertyRef storage,
+ PropertyRef init) final {}
+ void deleteProperties(PropertyRef prop) final {}
void populateDefaultProperties(OperationName opName,
- OpaqueProperties properties) final {}
+ PropertyRef properties) final {}
LogicalResult
- setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
+ setPropertiesFromAttr(OperationName opName, PropertyRef properties,
Attribute attr,
function_ref<InFlightDiagnostic()> emitError) final {
emitError() << "extensible Dialects don't support properties";
return failure();
}
Attribute getPropertiesAsAttr(Operation *op) final { return {}; }
- void copyProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {}
- bool compareProperties(OpaqueProperties, OpaqueProperties) final {
- return true;
- }
- llvm::hash_code hashProperties(OpaqueProperties prop) final { return {}; }
+ void copyProperties(PropertyRef lhs, PropertyRef rhs) final {}
+ bool compareProperties(PropertyRef, PropertyRef) final { return true; }
+ llvm::hash_code hashProperties(PropertyRef prop) final { return {}; }
private:
DynamicOpDefinition(
diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index be92fe0a6c7e3..e886ac45675e2 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -243,9 +243,10 @@ class OpState {
/// so we can cast it away here.
explicit OpState(Operation *state) : state(state) {}
- /// For all op which don't have properties, we keep a single instance of
- /// `EmptyProperties` to be used where a reference to a properties is needed:
- /// this allow to bind a pointer to the reference without triggering UB.
+ /// For all ops which don't have properties, we keep a single instance of
+ /// `EmptyProperties` to be used where a pointer to a struct of properties
+ /// is needed: this allows binding a pointer to the reference without
+ /// triggering UB.
static EmptyProperties &getEmptyProperties() {
static EmptyProperties emptyProperties;
return emptyProperties;
@@ -1978,7 +1979,7 @@ class Op : public OpState, public Traits<ConcreteType>... {
if constexpr (!hasProperties())
return getEmptyProperties();
return *getOperation()
- ->getPropertiesStorageUnsafe()
+ ->getPropertiesStorage()
.template as<InferredProperties<T> *>();
}
@@ -2152,4 +2153,6 @@ struct DenseMapInfo<T,
};
} // namespace llvm
+MLIR_DECLARE_EXPLICIT_TYPE_ID(mlir::EmptyProperties)
+
#endif
diff --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h
index f2c0f354d0ddb..06c9695fc7cf7 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -92,17 +92,15 @@ class alignas(8) Operation final
/// necessary.
static Operation *create(Location location, OperationName name,
TypeRange resultTypes, ValueRange operands,
- NamedAttrList &&attributes,
- OpaqueProperties properties, BlockRange successors,
- unsigned numRegions);
+ NamedAttrList &&attributes, PropertyRef properties,
+ BlockRange successors, unsigned numRegions);
/// Create a new Operation with the specific fields. This constructor uses an
/// existing attribute dictionary to avoid uniquing a list of attributes.
static Operation *create(Location location, OperationName name,
TypeRange resultTypes, ValueRange operands,
- DictionaryAttr attributes,
- OpaqueProperties properties, BlockRange successors,
- unsigned numRegions);
+ DictionaryAttr attributes, PropertyRef properties,
+ BlockRange successors, unsigned numRegions);
/// Create a new Operation from the fields stored in `state`.
static Operation *create(const OperationState &state);
@@ -110,8 +108,7 @@ class alignas(8) Operation final
/// Create a new Operation with the specific fields.
static Operation *create(Location location, OperationName name,
TypeRange resultTypes, ValueRange operands,
- NamedAttrList &&attributes,
- OpaqueProperties properties,
+ NamedAttrList &&attributes, PropertyRef properties,
BlockRange successors = {},
RegionRange regions = {});
@@ -925,23 +922,29 @@ class alignas(8) Operation final
int getPropertiesStorageSize() const {
return ((int)propertiesStorageSize) * 8;
}
- /// Returns the properties storage.
- OpaqueProperties getPropertiesStorage() {
+
+ /// Return a generic (but typed) reference to the property type storage.
+ PropertyRef getPropertiesStorage() {
if (propertiesStorageSize)
- return getPropertiesStorageUnsafe();
+ return PropertyRef(name.getOpPropertiesTypeID(),
+ getRawPropertiesStorageUnsafe());
return {nullptr};
}
- OpaqueProperties getPropertiesStorage() const {
+
+ PropertyRef getPropertiesStorage() const {
if (propertiesStorageSize)
- return {reinterpret_cast<void *>(const_cast<detail::OpProperties *>(
- getTrailingObjects<detail::OpProperties>()))};
+ return PropertyRef(
+ name.getOpPropertiesTypeID(),
+ reinterpret_cast<void *>(const_cast<detail::OpProperties *>(
+ getTrailingObjects<detail::OpProperties>())));
return {nullptr};
}
- /// Returns the properties storage without checking whether properties are
- /// present.
- OpaqueProperties getPropertiesStorageUnsafe() {
- return {
- reinterpret_cast<void *>(getTrailingObjects<detail::OpProperties>())};
+
+ /// Returns a pointer to the properties storage (if it exists) with no type
+ /// information.
+ void *getRawPropertiesStorageUnsafe() {
+ return reinterpret_cast<void *>(const_cast<detail::OpProperties *>(
+ getTrailingObjects<detail::OpProperties>()));
}
/// Return the properties converted to an attribute.
@@ -961,7 +964,7 @@ class alignas(8) Operation final
/// Copy properties from an existing other properties object. The two objects
/// must be the same type.
- void copyProperties(OpaqueProperties rhs);
+ void copyProperties(PropertyRef rhs);
/// Compute a hash for the op properties (if any).
llvm::hash_code hashProperties();
@@ -990,7 +993,7 @@ class alignas(8) Operation final
Operation(Location location, OperationName name, unsigned numResults,
unsigned numSuccessors, unsigned numRegions,
int propertiesStorageSize, DictionaryAttr attributes,
- OpaqueProperties properties, bool hasOperandStorage);
+ PropertyRef properties, bool hasOperandStorage);
// Operations are deleted through the destroy() member because they are
// allocated with malloc.
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index ac069df1d3e7d..07898e9c90700 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -63,22 +63,34 @@ template <typename ValueRangeT>
class ValueTypeRange;
//===----------------------------------------------------------------------===//
-// OpaqueProperties
+// PropertyRef
//===----------------------------------------------------------------------===//
-/// Simple wrapper around a void* in order to express generically how to pass
-/// in op properties through APIs.
-class OpaqueProperties {
+/// Type-safe wrapper around a void* for passing properties, including the
+/// properties structs of operations, generically through APIs. Pairs data with
+/// a TypeID for assert-based type checking. Note that the type in the type ID
+/// is the **storage** type of the property, and that the default object has a
+/// null data pointer and a type ID equal to the type ID for `void`.
+class PropertyRef {
public:
- OpaqueProperties(void *prop) : properties(prop) {}
- operator bool() const { return properties != nullptr; }
+ PropertyRef() = default;
+ PropertyRef(std::nullptr_t) {}
+ PropertyRef(TypeID typeID, void *data) : typeID(typeID), data(data) {}
+ operator bool() const { return data != nullptr; }
template <typename Dest>
Dest as() const {
- return static_cast<Dest>(const_cast<void *>(properties));
+ static_assert(std::is_pointer_v<Dest>,
+ "PropertyRef::as<T>() requires T to be a pointer type");
+ using RawType = std::remove_cv_t<std::remove_pointer_t<Dest>>;
+ assert((typeID == TypeID::get<RawType>()) &&
+ "Property type mismatch: TypeID does not match requested type");
+ return static_cast<Dest>(data);
}
+ TypeID getTypeID() const { return typeID; }
private:
- void *properties;
+ TypeID typeID;
+ void *data = nullptr;
};
//===----------------------------------------------------------------------===//
@@ -130,18 +142,18 @@ class OperationName {
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
function_ref<InFlightDiagnostic()> emitError) = 0;
virtual int getOpPropertyByteSize() = 0;
- virtual void initProperties(OperationName opName, OpaqueProperties storage,
- OpaqueProperties init) = 0;
- virtual void deleteProperties(OpaqueProperties) = 0;
+ virtual void initProperties(OperationName opName, PropertyRef storage,
+ PropertyRef init) = 0;
+ virtual void deleteProperties(PropertyRef) = 0;
virtual void populateDefaultProperties(OperationName opName,
- OpaqueProperties properties) = 0;
+ PropertyRef properties) = 0;
virtual LogicalResult
- setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
+ setPropertiesFromAttr(OperationName, PropertyRef, Attribute,
function_ref<InFlightDiagnostic()> emitError) = 0;
virtual Attribute getPropertiesAsAttr(Operation *) = 0;
- virtual void copyProperties(OpaqueProperties, OpaqueProperties) = 0;
- virtual bool compareProperties(OpaqueProperties, OpaqueProperties) = 0;
- virtual llvm::hash_code hashProperties(OpaqueProperties) = 0;
+ virtual void copyProperties(PropertyRef, PropertyRef) = 0;
+ virtual bool compareProperties(PropertyRef, PropertyRef) = 0;
+ virtual llvm::hash_code hashProperties(PropertyRef) = 0;
};
public:
@@ -160,6 +172,7 @@ class OperationName {
Dialect *getDialect() const { return dialect; }
StringAttr getName() const { return name; }
TypeID getTypeID() const { return typeID; }
+ TypeID getPropertiesTypeID() const { return propertiesTypeID; }
ArrayRef<StringAttr> getAttributeNames() const { return attributeNames; }
protected:
@@ -186,13 +199,20 @@ class OperationName {
/// lookup/creation/etc., as opposed to raw strings.
ArrayRef<StringAttr> attributeNames;
+ /// The TypeID of the Properties struct for this operation.
+ TypeID propertiesTypeID;
+
friend class RegisteredOperationName;
};
protected:
/// Default implementation for unregistered operations.
struct UnregisteredOpModel : public Impl {
- using Impl::Impl;
+ UnregisteredOpModel(StringAttr name, Dialect *dialect, TypeID typeID,
+ detail::InterfaceMap interfaceMap)
+ : Impl(name, dialect, typeID, std::move(interfaceMap)) {
+ propertiesTypeID = TypeID::get<Attribute>();
+ }
LogicalResult foldHook(Operation *, ArrayRef<Attribute>,
SmallVectorImpl<OpFoldResult> &) final;
void getCanonicalizationPatterns(RewritePatternSet &, MLIRContext *) final;
@@ -211,18 +231,18 @@ class OperationName {
verifyInherentAttrs(OperationName opName, NamedAttrList &attributes,
function_ref<InFlightDiagnostic()> emitError) final;
int getOpPropertyByteSize() final;
- void initProperties(OperationName opName, OpaqueProperties storage,
- OpaqueProperties init) final;
- void deleteProperties(OpaqueProperties) final;
+ void initProperties(OperationName opName, PropertyRef storage,
+ PropertyRef init) final;
+ void deleteProperties(PropertyRef) final;
void populateDefaultProperties(OperationName opName,
- OpaqueProperties properties) final;
+ PropertyRef properties) final;
LogicalResult
- setPropertiesFromAttr(OperationName, OpaqueProperties, Attribute,
+ setPropertiesFromAttr(OperationName, PropertyRef, Attribute,
function_ref<InFlightDiagnostic()> emitError) final;
Attribute getPropertiesAsAttr(Operation *) final;
- void copyProperties(OpaqueProperties, OpaqueProperties) final;
- bool compareProperties(OpaqueProperties, OpaqueProperties) final;
- llvm::hash_code hashProperties(OpaqueProperties) final;
+ void copyProperties(PropertyRef, PropertyRef) final;
+ bool compareProperties(PropertyRef, PropertyRef) final;
+ llvm::hash_code hashProperties(PropertyRef) final;
};
public:
@@ -413,18 +433,23 @@ class OperationName {
return getImpl()->getOpPropertyByteSize();
}
+ /// Return the TypeID of the op properties.
+ TypeID getOpPropertiesTypeID() const {
+ return getImpl()->getPropertiesTypeID();
+ }
+
/// This hooks destroy the op properties.
- void destroyOpProperties(OpaqueProperties properties) const {
+ void destroyOpProperties(PropertyRef properties) const {
getImpl()->deleteProperties(properties);
}
/// Initialize the op properties.
- void initOpProperties(OpaqueProperties storage, OpaqueProperties init) const {
+ void initOpProperties(PropertyRef storage, PropertyRef init) const {
getImpl()->initProperties(*this, storage, init);
}
/// Set the default values on the ODS attribute in the properties.
- void populateDefaultProperties(OpaqueProperties properties) const {
+ void populateDefaultProperties(PropertyRef properties) const {
getImpl()->populateDefaultProperties(*this, properties);
}
@@ -435,21 +460,21 @@ class OperationName {
/// Define the op properties from the provided Attribute.
LogicalResult setOpPropertiesFromAttribute(
- OperationName opName, OpaqueProperties properties, Attribute attr,
+ OperationName opName, PropertyRef properties, Attribute attr,
function_ref<InFlightDiagnostic()> emitError) const {
return getImpl()->setPropertiesFromAttr(opName, properties, attr,
emitError);
}
- void copyOpProperties(OpaqueProperties lhs, OpaqueProperties rhs) const {
+ void copyOpProperties(PropertyRef lhs, PropertyRef rhs) const {
return getImpl()->copyProperties(lhs, rhs);
}
- bool compareOpProperties(OpaqueProperties lhs, OpaqueProperties rhs) const {
+ bool compareOpProperties(PropertyRef lhs, PropertyRef rhs) const {
return getImpl()->compareProperties(lhs, rhs);
}
- llvm::hash_code hashOpProperties(OpaqueProperties properties) const {
+ llvm::hash_code hashOpProperties(PropertyRef properties) const {
return getImpl()->hashProperties(properties);
}
@@ -528,9 +553,13 @@ class RegisteredOperationName : public OperationName {
/// to a concrete op implementation.
template <typename ConcreteOp>
struct Model : public Impl {
+ using Properties = std::remove_reference_t<
+ decltype(std::declval<ConcreteOp>().getProperties())>;
Model(Dialect *dialect)
: Impl(ConcreteOp::getOperationName(), dialect,
- TypeID::get<ConcreteOp>(), ConcreteOp::getInterfaceMap()) {}
+ TypeID::get<ConcreteOp>(), ConcreteOp::getInterfaceMap()) {
+ propertiesTypeID = TypeID::get<Properties>();
+ }
LogicalResult foldHook(Operation *op, ArrayRef<Attribute> attrs,
SmallVectorImpl<OpFoldResult> &results) final {
return ConcreteOp::getFoldHookFn()(op, attrs, results);
@@ -560,9 +589,6 @@ class RegisteredOperationName : public OperationName {
/// Implementation for "Properties"
- using Properties = std::remove_reference_t<
- decltype(std::declval<ConcreteOp>().getProperties())>;
-
std::optional<Attribute> getInherentAttr(Operation *op,
StringRef name) final {
if constexpr (hasProperties) {
@@ -606,8 +632,8 @@ class RegisteredOperationName : public OperationName {
return sizeof(Properties);
return 0;
}
- void initProperties(OperationName opName, OpaqueProperties storage,
- OpaqueProperties init) final {
+ void initProperties(OperationName opName, PropertyRef storage,
+ PropertyRef init) final {
using Properties =
typename ConcreteOp::template InferredProperties<ConcreteOp>;
if (init)
@@ -618,18 +644,18 @@ class RegisteredOperationName : public OperationName {
ConcreteOp::populateDefaultProperties(opName,
*storage.as<Properties *>());
}
- void deleteProperties(OpaqueProperties prop) final {
+ void deleteProperties(PropertyRef prop) final {
prop.as<Properties *>()->~Properties();
}
void populateDefaultProperties(OperationName opName,
- OpaqueProperties properties) final {
+ PropertyRef properties) final {
if constexpr (hasProperties)
ConcreteOp::populateDefaultProperties(opName,
*properties.as<Properties *>());
}
LogicalResult
- setPropertiesFromAttr(OperationName opName, OpaqueProperties properties,
+ setPropertiesFromAttr(OperationName opName, PropertyRef properties,
Attribute attr,
function_ref<InFlightDiagnostic()> emitError) final {
if constexpr (hasProperties) {
@@ -647,15 +673,15 @@ class RegisteredOperationName : public OperationName {
}
return {};
}
- bool compareProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {
+ bool compareProperties(PropertyRef lhs, PropertyRef rhs) final {
if constexpr (hasProperties)
return *lhs.as<Properties *>() == *rhs.as<Properties *>();
return true;
}
- void copyProperties(OpaqueProperties lhs, OpaqueProperties rhs) final {
+ void copyProperties(PropertyRef lhs, PropertyRef rhs) final {
*lhs.as<Properties *>() = *rhs.as<Properties *>();
}
- llvm::hash_code hashProperties(OpaqueProperties prop) final {
+ llvm::hash_code hashProperties(PropertyRef prop) final {
if constexpr (hasProperties)
return ConcreteOp::computePropertiesHash(*prop.as<Properties *>());
@@ -959,11 +985,9 @@ struct OperationState {
Attribute propertiesAttr;
private:
- OpaqueProperties properties = nullptr;
- TypeID propertiesId;
- llvm::function_ref<void(OpaqueProperties)> propertiesDeleter;
- llvm::function_ref<void(OpaqueProperties, const OpaqueProperties)>
- propertiesSetter;
+ PropertyRef properties;
+ llvm::function_ref<void(PropertyRef)> propertiesDeleter;
+ llvm::function_ref<void(PropertyRef, const PropertyRef)> propertiesSetter;
friend class Operation;
public:
@@ -984,13 +1008,13 @@ struct Operatio...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/185157
More information about the Mlir-commits
mailing list