[MLIR][TableGen] Add genMnemonicAlias field for OpAsm{Type,Attr}Interface #131504
Conversation
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Hongren Zheng (ZenithalHourlyRate) ChangesSince the introduction of A common pattern is to just use the type/attr mnemonic as the alias. Previously, like #130479/#130481/#130483, this means adding a default implementation to This commit adds a For users wanting other alias behavior, they can ignore such field and still use One caveat is that to make it actually work, user like - list<Trait> traits = defTraits;
+ list<Trait> traits = defTraits #
+ !if(!eq(genMnemonicAlias, 1),
+ !if(!eq(valueType, "Attr"),
+ [NativeTrait<"OpAsmAttrInterface", "OpAsmAttrInterface">],
+ [NativeTrait<"OpAsmTypeInterface", "OpAsmTypeInterface">]),
+ []);but it did not work. I think the requirement of dialect users adding the Another question might be, why not making the default implementation in Full diff: https://github.com/llvm/llvm-project/pull/131504.diff 5 Files Affected:
diff --git a/mlir/include/mlir/IR/AttrTypeBase.td b/mlir/include/mlir/IR/AttrTypeBase.td
index 38d38cf098df3..7ea82879bfdcd 100644
--- a/mlir/include/mlir/IR/AttrTypeBase.td
+++ b/mlir/include/mlir/IR/AttrTypeBase.td
@@ -249,6 +249,9 @@ class AttrOrTypeDef<string valueType, string name, list<Trait> defTraits,
// generated code is placed inside the class's C++ namespace. `$cppClass` is
// replaced by the class name.
code extraClassDefinition = [{}];
+
+ // Generate a default 'getAlias' method for OpAsm{Type,Attr}Interface.
+ bit genMnemonicAlias = 0;
}
// Define a new attribute, named `name`, belonging to `dialect` that inherits
diff --git a/mlir/include/mlir/TableGen/AttrOrTypeDef.h b/mlir/include/mlir/TableGen/AttrOrTypeDef.h
index c3d730e42ef70..dd811557c065e 100644
--- a/mlir/include/mlir/TableGen/AttrOrTypeDef.h
+++ b/mlir/include/mlir/TableGen/AttrOrTypeDef.h
@@ -212,6 +212,10 @@ class AttrOrTypeDef {
/// Returns the def's extra class definition code.
std::optional<StringRef> getExtraDefs() const;
+ /// Returns true if we need to generate a default 'getAlias' implementation
+ /// using the mnemonic.
+ bool genMnemonicAlias() const;
+
/// Get the code location (for error printing).
ArrayRef<SMLoc> getLoc() const;
diff --git a/mlir/lib/TableGen/AttrOrTypeDef.cpp b/mlir/lib/TableGen/AttrOrTypeDef.cpp
index 9e8f789d71b5e..5f77e413c9368 100644
--- a/mlir/lib/TableGen/AttrOrTypeDef.cpp
+++ b/mlir/lib/TableGen/AttrOrTypeDef.cpp
@@ -205,6 +205,10 @@ std::optional<StringRef> AttrOrTypeDef::getExtraDefs() const {
return value.empty() ? std::optional<StringRef>() : value;
}
+bool AttrOrTypeDef::genMnemonicAlias() const {
+ return def->getValueAsBit("genMnemonicAlias");
+}
+
ArrayRef<SMLoc> AttrOrTypeDef::getLoc() const { return def->getLoc(); }
bool AttrOrTypeDef::skipDefaultBuilders() const {
diff --git a/mlir/test/mlir-tblgen/attrdefs.td b/mlir/test/mlir-tblgen/attrdefs.td
index 35d2c49619ee6..95a53a4aade48 100644
--- a/mlir/test/mlir-tblgen/attrdefs.td
+++ b/mlir/test/mlir-tblgen/attrdefs.td
@@ -172,3 +172,16 @@ def H_TestExtraClassAttr : TestAttr<"TestExtraClass"> {
// DEF-LABEL: int TestExtraClassAttr::getFoo(int i) {
// DEF: return i+1;
// DEF-NEXT: }
+
+def I_TestGenMnemonicAliasAttr : TestAttr<"TestGenMnemonicAlias"> {
+ let mnemonic = "test_gen_mnemonic_alias";
+ let genMnemonicAlias = 1;
+}
+
+// DECL-LABEL: class TestGenMnemonicAliasAttr : public ::mlir::Attribute
+// DECL: ::mlir::OpAsmAliasResult getAlias(::llvm::raw_ostream &os) const;
+
+// DEF-LABEL: ::mlir::OpAsmAliasResult TestGenMnemonicAliasAttr::getAlias(::llvm::raw_ostream &os) const {
+// DEF-NEXT: os << "test_gen_mnemonic_alias";
+// DEF-NEXT: return ::mlir::OpAsmAliasResult::OverridableAlias;
+// DEF-NEXT: }
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index 6a39424bd463f..ad6e3daa8fa40 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -130,6 +130,12 @@ class DefGen {
/// Emit a trait method.
void emitTraitMethod(const InterfaceMethod &method);
+ //===--------------------------------------------------------------------===//
+ // OpAsm{Type,Attr}Interface Default Method Emission
+
+ /// Emit 'getAlias' method using mnemonic as alias.
+ void emitMnemonicAliasMethod();
+
//===--------------------------------------------------------------------===//
// Storage Class Emission
void emitStorageClass();
@@ -215,6 +221,9 @@ DefGen::DefGen(const AttrOrTypeDef &def)
emitAccessors();
// Emit trait interface methods
emitInterfaceMethods();
+ // Emit OpAsm{Type,Attr}Interface default methods
+ if (def.genMnemonicAlias())
+ emitMnemonicAliasMethod();
defCls.finalize();
// Emit a storage class if one is needed
if (storageCls && def.genStorageClass())
@@ -576,6 +585,23 @@ void DefGen::emitTraitMethod(const InterfaceMethod &method) {
std::move(params));
}
+//===----------------------------------------------------------------------===//
+// OpAsm{Type,Attr}Interface Default Method Emission
+
+void DefGen::emitMnemonicAliasMethod() {
+ // If the mnemonic is not set, there is nothing to do.
+ if (!def.getMnemonic()) {
+ return;
+ }
+
+ // Emit the mnemonic alias method.
+ SmallVector<MethodParameter> params{{"::llvm::raw_ostream &", "os"}};
+ Method *m = defCls.addMethod<Method::Const>("::mlir::OpAsmAliasResult",
+ "getAlias", std::move(params));
+ m->body().indent() << strfmt("os << \"{0}\";\n", *def.getMnemonic())
+ << "return ::mlir::OpAsmAliasResult::OverridableAlias;\n";
+}
+
//===----------------------------------------------------------------------===//
// Storage Class Emission
|
There was a problem hiding this comment.
Nice addition! Two comments on the PR:
- When this field is used, we should auto-add the Asm interface the generated definition (we shouldn't require the user to do it)
- Please add documentation of this field to the documentation for attr/type definitions. docs/DefiningDialects/AttributesAndTypes.md
ZenithalHourlyRate
force-pushed
the
tblgen-alias
branch
from
517ced8 to
6b419c0
Compare
I still could not figure out how to do that with simple changes to tablegen files. Append the interface directly to AttrOrTypeDef trait list- list<Trait> traits = defTraits;
+ list<Trait> traits = defTraits #
+ !if(!eq(genMnemonicAlias, 1),
+ !if(!eq(valueType, "Attr"),
+ [OpAsmAttrInterface],
+ [OpAsmTypeInterface]),
+ []);This will form a cyclic dependency. Append the trait using NativeTrait- list<Trait> traits = defTraits;
+ list<Trait> traits = defTraits #
+ !if(!eq(genMnemonicAlias, 1),
+ !if(!eq(valueType, "Attr"),
+ [NativeAttrTrait<"OpAsmAttrInterface">],
+ [NativeTypeTrait<"OpAsmTypeInterface">]),
+ []);In generated file Append the trait manually- list<Trait> traits = defTraits;
+ list<Trait> traits = defTraits #
+ !if(!eq(genMnemonicAlias, 1),
+ !if(!eq(valueType, "Attr"),
+ [CustomTraitClass<"OpAsmAttrInterface::Trait">],
+ [CustomTraitClass<"OpAsmTypeInterface::Trait">]),
+ []);We could hack to make it work like get a Append it in
|
|
Sorry for the delay, the chrome extension I normally use for tracking reviews is dead with new chrome. For attaching the interface, I was expecting we'd just manually do it on the C++ side. We do the same thing for operations: |
ZenithalHourlyRate
force-pushed
the
tblgen-alias
branch
from
6b419c0 to
9e32afe
Compare
|
Changed Such change, aside from the test inside this PR, could be reflected in #130479 |
ZenithalHourlyRate
force-pushed
the
tblgen-alias
branch
from
9e32afe to
a6530cb
Compare
…ation (#130479) After the introduction of `OpAsmAttrInterface`, it is favorable to migrate code using `OpAsmDialectInterface` for ASM alias generation, which lives in `Dialect.cpp`, to use `OpAsmAttrInterface`, which lives in `Attrs.td`. In this way, attribute behavior is placed near its tablegen definition and people won't need to go through other files to know what other (unexpected) hooks comes into play. See #124721 for the interface itself and #128191 for prior migration for Builtin Attributes. See #131504 for the `genMnemonicAlias` tablegen field.
Since the introduction of
OpAsm{Type,Attr}Interface(#121187), it is possible to generate alias in AsmPrinter solely from the type/attribute itself without consulting theOpAsmDialectInterface. This means the behavior can be put in tablegen file near the type/attribute definition.A common pattern is to just use the type/attr mnemonic as the alias. Previously, like #130479/#130481/#130483, this means adding a default implementation to
extraClassDeclarationinLLVM_Attrbase class. However, as attribute definition may overrideextraClassDeclaration, it might be preferred to have a new field in tablegen to specify this behavior.This commit adds a
genMnemonicAliasfield toAttrOrTypeDef, when enabled, makesmlir-tblgenemit a default implementation ofgetAliasusing mnemonic. WhenOpAsm{Attr,Type}Interfaceis not specified by the user,tblgenwill automatically add the interface.For users wanting other alias behavior, they can ignore such field and still use
extraClassDeclarationway.