Skip to content

Commit 4b379de

Browse files
ZenithalHourlyRatejph-13
authored andcommitted
[mlir][NFC] Migrate to OpAsmAttrInterface for some Builtin Attributes 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.
1 parent c25ac7a commit 4b379de

7 files changed

+87
-44
lines changed

mlir/include/mlir/IR/BuiltinAttributeInterfaces.h

+1
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,7 @@ verifyAffineMapAsLayout(AffineMap m, ArrayRef<int64_t> shape,
279279
//===----------------------------------------------------------------------===//
280280

281281
#include "mlir/IR/BuiltinAttributeInterfaces.h.inc"
282+
#include "mlir/IR/OpAsmAttrInterface.h.inc"
282283

283284
//===----------------------------------------------------------------------===//
284285
// ElementsAttr

mlir/include/mlir/IR/BuiltinAttributes.td

+26-3
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ class Builtin_Attr<string name, string attrMnemonic, list<Trait> traits = [],
3737
//===----------------------------------------------------------------------===//
3838

3939
def Builtin_AffineMapAttr : Builtin_Attr<"AffineMap", "affine_map", [
40-
MemRefLayoutAttrInterface
40+
MemRefLayoutAttrInterface,
41+
OpAsmAttrInterface
4142
]> {
4243
let summary = "An Attribute containing an AffineMap object";
4344
let description = [{
@@ -63,6 +64,16 @@ def Builtin_AffineMapAttr : Builtin_Attr<"AffineMap", "affine_map", [
6364
let extraClassDeclaration = [{
6465
using ValueType = AffineMap;
6566
AffineMap getAffineMap() const { return getValue(); }
67+
68+
//===------------------------------------------------------------------===//
69+
// OpAsmAttrInterface Methods
70+
//===------------------------------------------------------------------===//
71+
72+
/// Get a name to use when generating an alias for this attribute.
73+
::mlir::OpAsmAliasResult getAlias(::llvm::raw_ostream &os) const {
74+
os << "map";
75+
return ::mlir::OpAsmAliasResult::OverridableAlias;
76+
}
6677
}];
6778
let skipDefaultBuilders = 1;
6879
}
@@ -755,7 +766,7 @@ def Builtin_IntegerAttr : Builtin_Attr<"Integer", "integer",
755766
// IntegerSetAttr
756767
//===----------------------------------------------------------------------===//
757768

758-
def Builtin_IntegerSetAttr : Builtin_Attr<"IntegerSet", "integer_set"> {
769+
def Builtin_IntegerSetAttr : Builtin_Attr<"IntegerSet", "integer_set", [OpAsmAttrInterface]> {
759770
let summary = "An Attribute containing an IntegerSet object";
760771
let description = [{
761772
Syntax:
@@ -776,7 +787,19 @@ def Builtin_IntegerSetAttr : Builtin_Attr<"IntegerSet", "integer_set"> {
776787
return $_get(value.getContext(), value);
777788
}]>
778789
];
779-
let extraClassDeclaration = "using ValueType = IntegerSet;";
790+
let extraClassDeclaration = [{
791+
using ValueType = IntegerSet;
792+
793+
//===------------------------------------------------------------------===//
794+
// OpAsmAttrInterface Methods
795+
//===------------------------------------------------------------------===//
796+
797+
/// Get a name to use when generating an alias for this attribute.
798+
::mlir::OpAsmAliasResult getAlias(::llvm::raw_ostream &os) const {
799+
os << "set";
800+
return ::mlir::OpAsmAliasResult::OverridableAlias;
801+
}
802+
}];
780803
let skipDefaultBuilders = 1;
781804
}
782805

mlir/include/mlir/IR/BuiltinTypeInterfaces.h

+2
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#ifndef MLIR_IR_BUILTINTYPEINTERFACES_H
1010
#define MLIR_IR_BUILTINTYPEINTERFACES_H
1111

12+
#include "mlir/IR/OpAsmSupport.h"
1213
#include "mlir/IR/Types.h"
1314

1415
namespace llvm {
@@ -21,5 +22,6 @@ class MLIRContext;
2122
} // namespace mlir
2223

2324
#include "mlir/IR/BuiltinTypeInterfaces.h.inc"
25+
#include "mlir/IR/OpAsmTypeInterface.h.inc"
2426

2527
#endif // MLIR_IR_BUILTINTYPEINTERFACES_H

mlir/include/mlir/IR/OpAsmInterface.td

+4-4
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,9 @@ def OpAsmTypeInterface : TypeInterface<"OpAsmTypeInterface"> {
130130
InterfaceMethod<[{
131131
Get a name to use when generating an alias for this type.
132132
}],
133-
"::mlir::OpAsmDialectInterface::AliasResult", "getAlias",
133+
"::mlir::OpAsmAliasResult", "getAlias",
134134
(ins "::llvm::raw_ostream&":$os), "",
135-
"return ::mlir::OpAsmDialectInterface::AliasResult::NoAlias;"
135+
"return ::mlir::OpAsmAliasResult::NoAlias;"
136136
>,
137137
];
138138
}
@@ -152,9 +152,9 @@ def OpAsmAttrInterface : AttrInterface<"OpAsmAttrInterface"> {
152152
InterfaceMethod<[{
153153
Get a name to use when generating an alias for this attribute.
154154
}],
155-
"::mlir::OpAsmDialectInterface::AliasResult", "getAlias",
155+
"::mlir::OpAsmAliasResult", "getAlias",
156156
(ins "::llvm::raw_ostream&":$os), "",
157-
"return ::mlir::OpAsmDialectInterface::AliasResult::NoAlias;"
157+
"return ::mlir::OpAsmAliasResult::NoAlias;"
158158
>,
159159
];
160160
}

mlir/include/mlir/IR/OpAsmSupport.h

+52
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
//===- OpAsmSupport.h - OpAsm Interface Utilities ---------------*- C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
//
9+
// This file defines various classes and utilites for
10+
// OpAsm{Dialect,Type,Attr,Op}Interface
11+
//
12+
//===----------------------------------------------------------------------===//
13+
14+
#ifndef MLIR_IR_OPASMSUPPORT_H_
15+
#define MLIR_IR_OPASMSUPPORT_H_
16+
17+
#include "mlir/IR/Block.h"
18+
#include "mlir/IR/Value.h"
19+
20+
namespace mlir {
21+
22+
//===--------------------------------------------------------------------===//
23+
// Utilities used by OpAsm{Dialect,Op,Type,Attr}Interface.
24+
//===--------------------------------------------------------------------===//
25+
26+
/// A functor used to set the name of the result. See 'getAsmResultNames' below
27+
/// for more details.
28+
using OpAsmSetNameFn = function_ref<void(StringRef)>;
29+
30+
/// A functor used to set the name of the start of a result group of an
31+
/// operation. See 'getAsmResultNames' below for more details.
32+
using OpAsmSetValueNameFn = function_ref<void(Value, StringRef)>;
33+
34+
/// A functor used to set the name of blocks in regions directly nested under
35+
/// an operation.
36+
using OpAsmSetBlockNameFn = function_ref<void(Block *, StringRef)>;
37+
38+
/// Holds the result of `OpAsm{Dialect,Attr,Type}Interface::getAlias` hook call.
39+
enum class OpAsmAliasResult {
40+
/// The object (type or attribute) is not supported by the hook
41+
/// and an alias was not provided.
42+
NoAlias,
43+
/// An alias was provided, but it might be overriden by other hook.
44+
OverridableAlias,
45+
/// An alias was provided and it should be used
46+
/// (no other hooks will be checked).
47+
FinalAlias
48+
};
49+
50+
} // namespace mlir
51+
52+
#endif // MLIR_IR_OPASMSUPPORT_H_

mlir/include/mlir/IR/OpImplementation.h

+2-29
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
#include "mlir/IR/BuiltinTypes.h"
1717
#include "mlir/IR/DialectInterface.h"
18+
#include "mlir/IR/OpAsmSupport.h"
1819
#include "mlir/IR/OpDefinition.h"
1920
#include "llvm/ADT/Twine.h"
2021
#include "llvm/Support/SMLoc.h"
@@ -1730,38 +1731,12 @@ class OpAsmParser : public AsmParser {
17301731
// Dialect OpAsm interface.
17311732
//===--------------------------------------------------------------------===//
17321733

1733-
/// A functor used to set the name of the result. See 'getAsmResultNames' below
1734-
/// for more details.
1735-
using OpAsmSetNameFn = function_ref<void(StringRef)>;
1736-
1737-
/// A functor used to set the name of the start of a result group of an
1738-
/// operation. See 'getAsmResultNames' below for more details.
1739-
using OpAsmSetValueNameFn = function_ref<void(Value, StringRef)>;
1740-
1741-
/// A functor used to set the name of blocks in regions directly nested under
1742-
/// an operation.
1743-
using OpAsmSetBlockNameFn = function_ref<void(Block *, StringRef)>;
1744-
17451734
class OpAsmDialectInterface
17461735
: public DialectInterface::Base<OpAsmDialectInterface> {
17471736
public:
17481737
OpAsmDialectInterface(Dialect *dialect) : Base(dialect) {}
17491738

1750-
//===------------------------------------------------------------------===//
1751-
// Aliases
1752-
//===------------------------------------------------------------------===//
1753-
1754-
/// Holds the result of `getAlias` hook call.
1755-
enum class AliasResult {
1756-
/// The object (type or attribute) is not supported by the hook
1757-
/// and an alias was not provided.
1758-
NoAlias,
1759-
/// An alias was provided, but it might be overriden by other hook.
1760-
OverridableAlias,
1761-
/// An alias was provided and it should be used
1762-
/// (no other hooks will be checked).
1763-
FinalAlias
1764-
};
1739+
using AliasResult = OpAsmAliasResult;
17651740

17661741
/// Hooks for getting an alias identifier alias for a given symbol, that is
17671742
/// not necessarily a part of this dialect. The identifier is used in place of
@@ -1827,9 +1802,7 @@ ParseResult parseDimensionList(OpAsmParser &parser,
18271802
//===--------------------------------------------------------------------===//
18281803

18291804
/// The OpAsmOpInterface, see OpAsmInterface.td for more details.
1830-
#include "mlir/IR/OpAsmAttrInterface.h.inc"
18311805
#include "mlir/IR/OpAsmOpInterface.h.inc"
1832-
#include "mlir/IR/OpAsmTypeInterface.h.inc"
18331806

18341807
namespace llvm {
18351808
template <>

mlir/lib/IR/BuiltinDialect.cpp

-8
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,6 @@ struct BuiltinOpAsmDialectInterface : public OpAsmDialectInterface {
4848
: OpAsmDialectInterface(dialect), blobManager(mgr) {}
4949

5050
AliasResult getAlias(Attribute attr, raw_ostream &os) const override {
51-
if (llvm::isa<AffineMapAttr>(attr)) {
52-
os << "map";
53-
return AliasResult::OverridableAlias;
54-
}
55-
if (llvm::isa<IntegerSetAttr>(attr)) {
56-
os << "set";
57-
return AliasResult::OverridableAlias;
58-
}
5951
if (llvm::isa<LocationAttr>(attr)) {
6052
os << "loc";
6153
return AliasResult::OverridableAlias;

0 commit comments

Comments
 (0)