-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[mlir][NFC] Migrate to OpAsmAttrInterface for some Builtin Attributes for alias #128191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][NFC] Migrate to OpAsmAttrInterface for some Builtin Attributes for alias #128191
Conversation
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-ods Author: Hongren Zheng (ZenithalHourlyRate) ChangesAfter the introduction of There is a However, the tricky part is that Note that only BuiltinAttribute/Type would face such issue as outside user can just include The dependency is introduced by the fact that There are two solutions to it
I currently take the first solution to demonstrate the dependency problem in this PR, both solution could be taken, and the place to put all these interfaces need some discussion. Full diff: https://github.com/llvm/llvm-project/pull/128191.diff 7 Files Affected:
diff --git a/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h b/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
index c4a42020d1389..982d35460ba41 100644
--- a/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
+++ b/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
@@ -12,6 +12,7 @@
#include "mlir/IR/AffineMap.h"
#include "mlir/IR/Attributes.h"
#include "mlir/IR/BuiltinTypeInterfaces.h"
+#include "mlir/IR/OpAsmDialectInterface.h"
#include "mlir/IR/Types.h"
#include "llvm/Support/raw_ostream.h"
#include <complex>
@@ -279,6 +280,7 @@ verifyAffineMapAsLayout(AffineMap m, ArrayRef<int64_t> shape,
//===----------------------------------------------------------------------===//
#include "mlir/IR/BuiltinAttributeInterfaces.h.inc"
+#include "mlir/IR/OpAsmAttrInterface.h.inc"
//===----------------------------------------------------------------------===//
// ElementsAttr
diff --git a/mlir/include/mlir/IR/BuiltinAttributes.h b/mlir/include/mlir/IR/BuiltinAttributes.h
index 901df3a25a46f..6155d0c65c67d 100644
--- a/mlir/include/mlir/IR/BuiltinAttributes.h
+++ b/mlir/include/mlir/IR/BuiltinAttributes.h
@@ -10,6 +10,7 @@
#define MLIR_IR_BUILTINATTRIBUTES_H
#include "mlir/IR/BuiltinAttributeInterfaces.h"
+#include "mlir/IR/OpAsmDialectInterface.h"
#include "llvm/ADT/APFloat.h"
#include "llvm/ADT/Sequence.h"
#include <complex>
diff --git a/mlir/include/mlir/IR/BuiltinAttributes.td b/mlir/include/mlir/IR/BuiltinAttributes.td
index 06f5e172a9909..2c86f92686e23 100644
--- a/mlir/include/mlir/IR/BuiltinAttributes.td
+++ b/mlir/include/mlir/IR/BuiltinAttributes.td
@@ -37,7 +37,8 @@ class Builtin_Attr<string name, string attrMnemonic, list<Trait> traits = [],
//===----------------------------------------------------------------------===//
def Builtin_AffineMapAttr : Builtin_Attr<"AffineMap", "affine_map", [
- MemRefLayoutAttrInterface
+ MemRefLayoutAttrInterface,
+ OpAsmAttrInterface
]> {
let summary = "An Attribute containing an AffineMap object";
let description = [{
@@ -63,6 +64,16 @@ def Builtin_AffineMapAttr : Builtin_Attr<"AffineMap", "affine_map", [
let extraClassDeclaration = [{
using ValueType = AffineMap;
AffineMap getAffineMap() const { return getValue(); }
+
+ //===------------------------------------------------------------------===//
+ // OpAsmAttrInterface Methods
+ //===------------------------------------------------------------------===//
+
+ /// Get a name to use when generating an alias for this attribute.
+ ::mlir::OpAsmDialectInterface::AliasResult getAlias(::llvm::raw_ostream &os) const {
+ os << "map";
+ return ::mlir::OpAsmDialectInterface::AliasResult::OverridableAlias;
+ }
}];
let skipDefaultBuilders = 1;
}
@@ -755,7 +766,7 @@ def Builtin_IntegerAttr : Builtin_Attr<"Integer", "integer",
// IntegerSetAttr
//===----------------------------------------------------------------------===//
-def Builtin_IntegerSetAttr : Builtin_Attr<"IntegerSet", "integer_set"> {
+def Builtin_IntegerSetAttr : Builtin_Attr<"IntegerSet", "integer_set", [OpAsmAttrInterface]> {
let summary = "An Attribute containing an IntegerSet object";
let description = [{
Syntax:
@@ -776,7 +787,19 @@ def Builtin_IntegerSetAttr : Builtin_Attr<"IntegerSet", "integer_set"> {
return $_get(value.getContext(), value);
}]>
];
- let extraClassDeclaration = "using ValueType = IntegerSet;";
+ let extraClassDeclaration = [{
+ using ValueType = IntegerSet;
+
+ //===------------------------------------------------------------------===//
+ // OpAsmAttrInterface Methods
+ //===------------------------------------------------------------------===//
+
+ /// Get a name to use when generating an alias for this attribute.
+ ::mlir::OpAsmDialectInterface::AliasResult getAlias(::llvm::raw_ostream &os) const {
+ os << "set";
+ return ::mlir::OpAsmDialectInterface::AliasResult::OverridableAlias;
+ }
+ }];
let skipDefaultBuilders = 1;
}
diff --git a/mlir/include/mlir/IR/BuiltinTypeInterfaces.h b/mlir/include/mlir/IR/BuiltinTypeInterfaces.h
index e8011b5488dc9..5851c624635bf 100644
--- a/mlir/include/mlir/IR/BuiltinTypeInterfaces.h
+++ b/mlir/include/mlir/IR/BuiltinTypeInterfaces.h
@@ -9,6 +9,7 @@
#ifndef MLIR_IR_BUILTINTYPEINTERFACES_H
#define MLIR_IR_BUILTINTYPEINTERFACES_H
+#include "mlir/IR/OpAsmDialectInterface.h"
#include "mlir/IR/Types.h"
namespace llvm {
@@ -21,5 +22,6 @@ class MLIRContext;
} // namespace mlir
#include "mlir/IR/BuiltinTypeInterfaces.h.inc"
+#include "mlir/IR/OpAsmTypeInterface.h.inc"
#endif // MLIR_IR_BUILTINTYPEINTERFACES_H
diff --git a/mlir/include/mlir/IR/OpAsmDialectInterface.h b/mlir/include/mlir/IR/OpAsmDialectInterface.h
new file mode 100644
index 0000000000000..9965d858daae4
--- /dev/null
+++ b/mlir/include/mlir/IR/OpAsmDialectInterface.h
@@ -0,0 +1,177 @@
+//===- OpAsmDialectInterface.h - OpAsm Dialect Interface --------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_IR_OPASMDIALECTINTERFACE_H
+#define MLIR_IR_OPASMDIALECTINTERFACE_H
+
+#include "mlir/IR/DialectInterface.h"
+#include "mlir/IR/Types.h"
+#include "mlir/IR/Value.h"
+
+namespace mlir {
+class AsmParsedResourceEntry;
+class AsmResourceBuilder;
+
+//===----------------------------------------------------------------------===//
+// AsmDialectResourceHandle
+//===----------------------------------------------------------------------===//
+
+/// This class represents an opaque handle to a dialect resource entry.
+class AsmDialectResourceHandle {
+public:
+ AsmDialectResourceHandle() = default;
+ AsmDialectResourceHandle(void *resource, TypeID resourceID, Dialect *dialect)
+ : resource(resource), opaqueID(resourceID), dialect(dialect) {}
+ bool operator==(const AsmDialectResourceHandle &other) const {
+ return resource == other.resource;
+ }
+
+ /// Return an opaque pointer to the referenced resource.
+ void *getResource() const { return resource; }
+
+ /// Return the type ID of the resource.
+ TypeID getTypeID() const { return opaqueID; }
+
+ /// Return the dialect that owns the resource.
+ Dialect *getDialect() const { return dialect; }
+
+private:
+ /// The opaque handle to the dialect resource.
+ void *resource = nullptr;
+ /// The type of the resource referenced.
+ TypeID opaqueID;
+ /// The dialect owning the given resource.
+ Dialect *dialect;
+};
+
+/// This class represents a CRTP base class for dialect resource handles. It
+/// abstracts away various utilities necessary for defined derived resource
+/// handles.
+template <typename DerivedT, typename ResourceT, typename DialectT>
+class AsmDialectResourceHandleBase : public AsmDialectResourceHandle {
+public:
+ using Dialect = DialectT;
+
+ /// Construct a handle from a pointer to the resource. The given pointer
+ /// should be guaranteed to live beyond the life of this handle.
+ AsmDialectResourceHandleBase(ResourceT *resource, DialectT *dialect)
+ : AsmDialectResourceHandle(resource, TypeID::get<DerivedT>(), dialect) {}
+ AsmDialectResourceHandleBase(AsmDialectResourceHandle handle)
+ : AsmDialectResourceHandle(handle) {
+ assert(handle.getTypeID() == TypeID::get<DerivedT>());
+ }
+
+ /// Return the resource referenced by this handle.
+ ResourceT *getResource() {
+ return static_cast<ResourceT *>(AsmDialectResourceHandle::getResource());
+ }
+ const ResourceT *getResource() const {
+ return const_cast<AsmDialectResourceHandleBase *>(this)->getResource();
+ }
+
+ /// Return the dialect that owns the resource.
+ DialectT *getDialect() const {
+ return static_cast<DialectT *>(AsmDialectResourceHandle::getDialect());
+ }
+
+ /// Support llvm style casting.
+ static bool classof(const AsmDialectResourceHandle *handle) {
+ return handle->getTypeID() == TypeID::get<DerivedT>();
+ }
+};
+
+inline llvm::hash_code hash_value(const AsmDialectResourceHandle ¶m) {
+ return llvm::hash_value(param.getResource());
+}
+
+//===--------------------------------------------------------------------===//
+// Dialect OpAsm interface.
+//===--------------------------------------------------------------------===//
+
+/// A functor used to set the name of the result. See 'getAsmResultNames' below
+/// for more details.
+using OpAsmSetNameFn = function_ref<void(StringRef)>;
+
+/// A functor used to set the name of the start of a result group of an
+/// operation. See 'getAsmResultNames' below for more details.
+using OpAsmSetValueNameFn = function_ref<void(Value, StringRef)>;
+
+/// A functor used to set the name of blocks in regions directly nested under
+/// an operation.
+using OpAsmSetBlockNameFn = function_ref<void(Block *, StringRef)>;
+
+class OpAsmDialectInterface
+ : public DialectInterface::Base<OpAsmDialectInterface> {
+public:
+ OpAsmDialectInterface(Dialect *dialect) : Base(dialect) {}
+
+ //===------------------------------------------------------------------===//
+ // Aliases
+ //===------------------------------------------------------------------===//
+
+ /// Holds the result of `getAlias` hook call.
+ enum class AliasResult {
+ /// The object (type or attribute) is not supported by the hook
+ /// and an alias was not provided.
+ NoAlias,
+ /// An alias was provided, but it might be overriden by other hook.
+ OverridableAlias,
+ /// An alias was provided and it should be used
+ /// (no other hooks will be checked).
+ FinalAlias
+ };
+
+ /// Hooks for getting an alias identifier alias for a given symbol, that is
+ /// not necessarily a part of this dialect. The identifier is used in place of
+ /// the symbol when printing textual IR. These aliases must not contain `.` or
+ /// end with a numeric digit([0-9]+).
+ virtual AliasResult getAlias(Attribute attr, raw_ostream &os) const {
+ return AliasResult::NoAlias;
+ }
+ virtual AliasResult getAlias(Type type, raw_ostream &os) const {
+ return AliasResult::NoAlias;
+ }
+
+ //===--------------------------------------------------------------------===//
+ // Resources
+ //===--------------------------------------------------------------------===//
+
+ /// Declare a resource with the given key, returning a handle to use for any
+ /// references of this resource key within the IR during parsing. The result
+ /// of `getResourceKey` on the returned handle is permitted to be different
+ /// than `key`.
+ virtual FailureOr<AsmDialectResourceHandle>
+ declareResource(StringRef key) const {
+ return failure();
+ }
+
+ /// Return a key to use for the given resource. This key should uniquely
+ /// identify this resource within the dialect.
+ virtual std::string
+ getResourceKey(const AsmDialectResourceHandle &handle) const {
+ llvm_unreachable(
+ "Dialect must implement `getResourceKey` when defining resources");
+ }
+
+ /// Hook for parsing resource entries. Returns failure if the entry was not
+ /// valid, or could otherwise not be processed correctly. Any necessary errors
+ /// can be emitted via the provided entry.
+ virtual LogicalResult parseResource(AsmParsedResourceEntry &entry) const;
+
+ /// Hook for building resources to use during printing. The given `op` may be
+ /// inspected to help determine what information to include.
+ /// `referencedResources` contains all of the resources detected when printing
+ /// 'op'.
+ virtual void
+ buildResources(Operation *op,
+ const SetVector<AsmDialectResourceHandle> &referencedResources,
+ AsmResourceBuilder &builder) const {}
+};
+} // namespace mlir
+
+#endif
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index d80bff2d78217..1e7ab6ac4575e 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -15,88 +15,15 @@
#include "mlir/IR/BuiltinTypes.h"
#include "mlir/IR/DialectInterface.h"
+#include "mlir/IR/OpAsmDialectInterface.h"
#include "mlir/IR/OpDefinition.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/SMLoc.h"
#include <optional>
namespace mlir {
-class AsmParsedResourceEntry;
-class AsmResourceBuilder;
class Builder;
-//===----------------------------------------------------------------------===//
-// AsmDialectResourceHandle
-//===----------------------------------------------------------------------===//
-
-/// This class represents an opaque handle to a dialect resource entry.
-class AsmDialectResourceHandle {
-public:
- AsmDialectResourceHandle() = default;
- AsmDialectResourceHandle(void *resource, TypeID resourceID, Dialect *dialect)
- : resource(resource), opaqueID(resourceID), dialect(dialect) {}
- bool operator==(const AsmDialectResourceHandle &other) const {
- return resource == other.resource;
- }
-
- /// Return an opaque pointer to the referenced resource.
- void *getResource() const { return resource; }
-
- /// Return the type ID of the resource.
- TypeID getTypeID() const { return opaqueID; }
-
- /// Return the dialect that owns the resource.
- Dialect *getDialect() const { return dialect; }
-
-private:
- /// The opaque handle to the dialect resource.
- void *resource = nullptr;
- /// The type of the resource referenced.
- TypeID opaqueID;
- /// The dialect owning the given resource.
- Dialect *dialect;
-};
-
-/// This class represents a CRTP base class for dialect resource handles. It
-/// abstracts away various utilities necessary for defined derived resource
-/// handles.
-template <typename DerivedT, typename ResourceT, typename DialectT>
-class AsmDialectResourceHandleBase : public AsmDialectResourceHandle {
-public:
- using Dialect = DialectT;
-
- /// Construct a handle from a pointer to the resource. The given pointer
- /// should be guaranteed to live beyond the life of this handle.
- AsmDialectResourceHandleBase(ResourceT *resource, DialectT *dialect)
- : AsmDialectResourceHandle(resource, TypeID::get<DerivedT>(), dialect) {}
- AsmDialectResourceHandleBase(AsmDialectResourceHandle handle)
- : AsmDialectResourceHandle(handle) {
- assert(handle.getTypeID() == TypeID::get<DerivedT>());
- }
-
- /// Return the resource referenced by this handle.
- ResourceT *getResource() {
- return static_cast<ResourceT *>(AsmDialectResourceHandle::getResource());
- }
- const ResourceT *getResource() const {
- return const_cast<AsmDialectResourceHandleBase *>(this)->getResource();
- }
-
- /// Return the dialect that owns the resource.
- DialectT *getDialect() const {
- return static_cast<DialectT *>(AsmDialectResourceHandle::getDialect());
- }
-
- /// Support llvm style casting.
- static bool classof(const AsmDialectResourceHandle *handle) {
- return handle->getTypeID() == TypeID::get<DerivedT>();
- }
-};
-
-inline llvm::hash_code hash_value(const AsmDialectResourceHandle ¶m) {
- return llvm::hash_value(param.getResource());
-}
-
//===----------------------------------------------------------------------===//
// AsmPrinter
//===----------------------------------------------------------------------===//
@@ -1726,90 +1653,6 @@ class OpAsmParser : public AsmParser {
SmallVectorImpl<UnresolvedOperand> &rhs) = 0;
};
-//===--------------------------------------------------------------------===//
-// Dialect OpAsm interface.
-//===--------------------------------------------------------------------===//
-
-/// A functor used to set the name of the result. See 'getAsmResultNames' below
-/// for more details.
-using OpAsmSetNameFn = function_ref<void(StringRef)>;
-
-/// A functor used to set the name of the start of a result group of an
-/// operation. See 'getAsmResultNames' below for more details.
-using OpAsmSetValueNameFn = function_ref<void(Value, StringRef)>;
-
-/// A functor used to set the name of blocks in regions directly nested under
-/// an operation.
-using OpAsmSetBlockNameFn = function_ref<void(Block *, StringRef)>;
-
-class OpAsmDialectInterface
- : public DialectInterface::Base<OpAsmDialectInterface> {
-public:
- OpAsmDialectInterface(Dialect *dialect) : Base(dialect) {}
-
- //===------------------------------------------------------------------===//
- // Aliases
- //===------------------------------------------------------------------===//
-
- /// Holds the result of `getAlias` hook call.
- enum class AliasResult {
- /// The object (type or attribute) is not supported by the hook
- /// and an alias was not provided.
- NoAlias,
- /// An alias was provided, but it might be overriden by other hook.
- OverridableAlias,
- /// An alias was provided and it should be used
- /// (no other hooks will be checked).
- FinalAlias
- };
-
- /// Hooks for getting an alias identifier alias for a given symbol, that is
- /// not necessarily a part of this dialect. The identifier is used in place of
- /// the symbol when printing textual IR. These aliases must not contain `.` or
- /// end with a numeric digit([0-9]+).
- virtual AliasResult getAlias(Attribute attr, raw_ostream &os) const {
- return AliasResult::NoAlias;
- }
- virtual AliasResult getAlias(Type type, raw_ostream &os) const {
- return AliasResult::NoAlias;
- }
-
- //===--------------------------------------------------------------------===//
- // Resources
- //===--------------------------------------------------------------------===//
-
- /// Declare a resource with the given key, returning a handle to use for any
- /// references of this resource key within the IR during parsing. The result
- /// of `getResourceKey` on the returned handle is permitted to be different
- /// than `key`.
- virtual FailureOr<AsmDialectResourceHandle>
- declareResource(StringRef key) const {
- return failure();
- }
-
- /// Return a key to use for the given resource. This key should uniquely
- /// identify this resource within the dialect.
- virtual std::string
- getResourceKey(const AsmDialectResourceHandle &handle) const {
- llvm_unreachable(
- "Dialect must implement `getResourceKey` when defining resources");
- }
-
- /// Hook for parsing resource entries. Returns failure if the entry was not
- /// valid, or could otherwise not be processed correctly. Any necessary errors
- /// can be emitted via the provided entry.
- virtual LogicalResult parseResource(AsmParsedResourceEntry &entry) const;
-
- /// Hook for building resources to use during printing. The given `op` may be
- /// inspected to help determine what information to include.
- /// `referencedResources` contains all of the resources detected when printing
- /// 'op'.
- virtual void
- buildResources(Operation *op,
- const SetVector<AsmDialectResourceHandle> &referencedResources,
- AsmResourceBuilder &builder) const {}
-};
-
//===--------------------------------------------------------------------===//
// Custom printers and parsers.
//===--------------------------------------------------------------------===//
@@ -1827,9 +1670,7 @@ ParseResult parseDimensionList(OpAsmParser &parser,
//===--------------------------------------------------------------------===//
/// The OpAsmOpInterface, see OpAsmInterface.td for more details.
-#include "mlir/IR/OpAsmAttrInterface.h.inc"
#include "mlir/IR/OpAsmOpInterface.h.inc"
-#include "mlir/IR/OpAsmTypeInterface.h.inc"
namespace llvm {
template <>
diff --git a/mlir/lib/IR/BuiltinDialect.cpp b/mlir/lib/IR/BuiltinDialect.cpp
index 99796c5f1c371..2867d9b4ef68a 100644
--- a/mlir/lib/IR/BuiltinDialect.cpp
+++ b/mlir/lib/IR/BuiltinDialect.cpp
@@ -48,14 +48,6 @@ struct BuiltinOpAsmDialectInterface : public OpAsmDialectInterface {
: OpAsmDialectInterface(dialect), blobManager(mgr) {}
AliasResult getAlias(Attribute attr, raw_ostream &os) const override {
- if (llvm::isa<AffineMapAttr>(attr)) {
- os << "map";
- return AliasResult::OverridableAlias;
- }
- if (llvm::isa<IntegerSetAttr>(attr)) {
- os << "set";
- return AliasResult::OverridableAlias;
- }
if (llvm::isa<LocationAttr>(attr)) {
os << "loc";
return AliasResult::OverridableAlias;
|
63c9273
to
3bdd21c
Compare
ping for review |
1 similar comment
ping for review |
Is this NFC? (I don't see any test updated) |
It is NFC with regard to the functionality of AffineAttr/IntegerSetAttr alias generation and OpAsm{}Interface definition. I am uncertain about the header file restructure though. |
My litmus test for NFC is: "if this isn't NFC then please provide a test that shows the behavior change" :) |
I think that LGTM overall, but the description does not seem to match the implementation, does it? Also can you cleanup the description, we don't need the commit history to have the description of non-implemented solutions. |
Changed the title with NFC and description. |
… for alias (llvm#128191) After the introduction of `OpAsmAttrInterface` for alias in llvm#124721, the natural thought to exercise it would be migrating the MLIR existing alias generation method, i.e. `OpAsmDialectInterface`, to use the new interface. There is a `BuiltinOpAsmDialectInterface` that generates aliases for `AffineMapAttr` and `IntegerSetAttr`, and these attributes could be migrated to use `OpAsmAttrInterface`. However, the tricky part is that `OpAsmAttrInterface` lives in `OpImplementation.h`. If `BuiltinAttributes.h` includes that, it would become a cyclic inclusion. Note that only BuiltinAttribute/Type would face such issue as outside user can just include `OpImplementation.h` (see downstream example google/heir#1437) The dependency is introduced by the fact that `OpAsmAttrInterface` uses `OpAsmDialectInterface::AliasResult`. The solution to is: Put the `AliasResult` in `OpAsmSupport.h` that all interfaces can include that header safely. The API wont break as `mlir::OpAsmDialectInterface::AliasResult` is a typedef of this class.
After the introduction of
OpAsmAttrInterface
for alias in #124721, the natural thought to exercise it would be migrating the MLIR existing alias generation method, i.e.OpAsmDialectInterface
, to use the new interface.There is a
BuiltinOpAsmDialectInterface
that generates aliases forAffineMapAttr
andIntegerSetAttr
, and these attributes could be migrated to useOpAsmAttrInterface
.However, the tricky part is that
OpAsmAttrInterface
lives inOpImplementation.h
. IfBuiltinAttributes.h
includes that, it would become a cyclic inclusion.Note that only BuiltinAttribute/Type would face such issue as outside user can just include
OpImplementation.h
(see downstream example google/heir#1437)The dependency is introduced by the fact that
OpAsmAttrInterface
usesOpAsmDialectInterface::AliasResult
.The solution to is: Put the
AliasResult
inOpAsmSupport.h
that all interfaces can include that header safely. The API wont break asmlir::OpAsmDialectInterface::AliasResult
is a typedef of this class.