Skip to content

[MLIR][TableGen] Add gen-attrdef-list #126332

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

Closed
wants to merge 1 commit into from

Conversation

andykaylor
Copy link
Contributor

This adds a new mlir-tblgen option (-gen-attrdef-list) to generate a list of AttrDefs from a .td file in a format suitable for use in patterns where a macro is defined to expand various repeated code snippets for each item in the list. Specifically, the file will contain a list in this format

ATTRDEF(MyAttr)

This will be used in ClangIR to create an attribute visitor.

This adds a new mlir-tblgen option (-gen-attrdef-list) to generate a list
of AttrDefs from a .td file in a format suitable for use in patterns where
a macro is defined to expand various repeated code snippets for each item
in the list. Specifically, the file will contain a list in this format

ATTRDEF(MyAttr)

This will be used in ClangIR to create an attribute visitor.
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Feb 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 8, 2025

@llvm/pr-subscribers-mlir-core

Author: Andy Kaylor (andykaylor)

Changes

This adds a new mlir-tblgen option (-gen-attrdef-list) to generate a list of AttrDefs from a .td file in a format suitable for use in patterns where a macro is defined to expand various repeated code snippets for each item in the list. Specifically, the file will contain a list in this format

ATTRDEF(MyAttr)

This will be used in ClangIR to create an attribute visitor.


Full diff: https://github.com/llvm/llvm-project/pull/126332.diff

2 Files Affected:

  • (modified) mlir/test/mlir-tblgen/attrdefs.td (+7)
  • (modified) mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp (+24-1)
diff --git a/mlir/test/mlir-tblgen/attrdefs.td b/mlir/test/mlir-tblgen/attrdefs.td
index 35d2c49619ee69c..ecb1c93108597ee 100644
--- a/mlir/test/mlir-tblgen/attrdefs.td
+++ b/mlir/test/mlir-tblgen/attrdefs.td
@@ -1,5 +1,6 @@
 // RUN: mlir-tblgen -gen-attrdef-decls -I %S/../../include %s | FileCheck %s --check-prefix=DECL
 // RUN: mlir-tblgen -gen-attrdef-defs -I %S/../../include %s | FileCheck %s --check-prefix=DEF
+// RUN: mlir-tblgen -gen-attrdef-list -I %S/../../include %s | FileCheck %s --check-prefix=LIST
 
 include "mlir/IR/AttrTypeBase.td"
 include "mlir/IR/OpBase.td"
@@ -19,6 +20,12 @@ include "mlir/IR/OpBase.td"
 // DEF: ::test::CompoundAAttr,
 // DEF: ::test::SingleParameterAttr
 
+// LIST: ATTRDEF(IndexAttr)
+// LIST: ATTRDEF(SimpleAAttr)
+// LIST: ATTRDEF(CompoundAAttr)
+// LIST: ATTRDEF(SingleParameterAttr)
+
+// LIST: #undef ATTRDEF
 // DEF-LABEL: ::mlir::OptionalParseResult generatedAttributeParser(
 // DEF-SAME: ::mlir::AsmParser &parser,
 // DEF-SAME: ::llvm::StringRef *mnemonic, ::mlir::Type type,
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index 6a39424bd463fd7..a5a9a5c55908788 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -690,6 +690,7 @@ class DefGenerator {
 public:
   bool emitDecls(StringRef selectedDialect);
   bool emitDefs(StringRef selectedDialect);
+  bool emitList(StringRef selectedDialect);
 
 protected:
   DefGenerator(ArrayRef<const Record *> defs, raw_ostream &os,
@@ -1025,6 +1026,23 @@ bool DefGenerator::emitDefs(StringRef selectedDialect) {
   return false;
 }
 
+bool DefGenerator::emitList(StringRef selectedDialect) {
+  emitSourceFileHeader(("List of " + defType + "Def Definitions").str(), os);
+
+  SmallVector<AttrOrTypeDef, 16> defs;
+  collectAllDefs(selectedDialect, defRecords, defs);
+  if (defs.empty())
+    return false;
+
+  auto interleaveFn = [&](const AttrOrTypeDef &def) {
+    os << defType.upper() << "DEF(" << def.getCppClassName() << ")";
+  };
+  llvm::interleave(defs, os, interleaveFn, "\n");
+  os << "\n\n";
+  os << "#undef " << defType.upper() << "DEF" << "\n";
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Type Constraints
 //===----------------------------------------------------------------------===//
@@ -1099,7 +1117,12 @@ static mlir::GenRegistration
                    AttrDefGenerator generator(records, os);
                    return generator.emitDecls(attrDialect);
                  });
-
+static mlir::GenRegistration
+    genAttrList("gen-attrdef-list", "Generate an AttrDef list",
+                [](const RecordKeeper &records, raw_ostream &os) {
+                  AttrDefGenerator generator(records, os);
+                  return generator.emitList(attrDialect);
+                });
 //===----------------------------------------------------------------------===//
 // TypeDef
 

@llvmbot
Copy link
Member

llvmbot commented Feb 8, 2025

@llvm/pr-subscribers-mlir

Author: Andy Kaylor (andykaylor)

Changes

This adds a new mlir-tblgen option (-gen-attrdef-list) to generate a list of AttrDefs from a .td file in a format suitable for use in patterns where a macro is defined to expand various repeated code snippets for each item in the list. Specifically, the file will contain a list in this format

ATTRDEF(MyAttr)

This will be used in ClangIR to create an attribute visitor.


Full diff: https://github.com/llvm/llvm-project/pull/126332.diff

2 Files Affected:

  • (modified) mlir/test/mlir-tblgen/attrdefs.td (+7)
  • (modified) mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp (+24-1)
diff --git a/mlir/test/mlir-tblgen/attrdefs.td b/mlir/test/mlir-tblgen/attrdefs.td
index 35d2c49619ee69c..ecb1c93108597ee 100644
--- a/mlir/test/mlir-tblgen/attrdefs.td
+++ b/mlir/test/mlir-tblgen/attrdefs.td
@@ -1,5 +1,6 @@
 // RUN: mlir-tblgen -gen-attrdef-decls -I %S/../../include %s | FileCheck %s --check-prefix=DECL
 // RUN: mlir-tblgen -gen-attrdef-defs -I %S/../../include %s | FileCheck %s --check-prefix=DEF
+// RUN: mlir-tblgen -gen-attrdef-list -I %S/../../include %s | FileCheck %s --check-prefix=LIST
 
 include "mlir/IR/AttrTypeBase.td"
 include "mlir/IR/OpBase.td"
@@ -19,6 +20,12 @@ include "mlir/IR/OpBase.td"
 // DEF: ::test::CompoundAAttr,
 // DEF: ::test::SingleParameterAttr
 
+// LIST: ATTRDEF(IndexAttr)
+// LIST: ATTRDEF(SimpleAAttr)
+// LIST: ATTRDEF(CompoundAAttr)
+// LIST: ATTRDEF(SingleParameterAttr)
+
+// LIST: #undef ATTRDEF
 // DEF-LABEL: ::mlir::OptionalParseResult generatedAttributeParser(
 // DEF-SAME: ::mlir::AsmParser &parser,
 // DEF-SAME: ::llvm::StringRef *mnemonic, ::mlir::Type type,
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index 6a39424bd463fd7..a5a9a5c55908788 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -690,6 +690,7 @@ class DefGenerator {
 public:
   bool emitDecls(StringRef selectedDialect);
   bool emitDefs(StringRef selectedDialect);
+  bool emitList(StringRef selectedDialect);
 
 protected:
   DefGenerator(ArrayRef<const Record *> defs, raw_ostream &os,
@@ -1025,6 +1026,23 @@ bool DefGenerator::emitDefs(StringRef selectedDialect) {
   return false;
 }
 
+bool DefGenerator::emitList(StringRef selectedDialect) {
+  emitSourceFileHeader(("List of " + defType + "Def Definitions").str(), os);
+
+  SmallVector<AttrOrTypeDef, 16> defs;
+  collectAllDefs(selectedDialect, defRecords, defs);
+  if (defs.empty())
+    return false;
+
+  auto interleaveFn = [&](const AttrOrTypeDef &def) {
+    os << defType.upper() << "DEF(" << def.getCppClassName() << ")";
+  };
+  llvm::interleave(defs, os, interleaveFn, "\n");
+  os << "\n\n";
+  os << "#undef " << defType.upper() << "DEF" << "\n";
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Type Constraints
 //===----------------------------------------------------------------------===//
@@ -1099,7 +1117,12 @@ static mlir::GenRegistration
                    AttrDefGenerator generator(records, os);
                    return generator.emitDecls(attrDialect);
                  });
-
+static mlir::GenRegistration
+    genAttrList("gen-attrdef-list", "Generate an AttrDef list",
+                [](const RecordKeeper &records, raw_ostream &os) {
+                  AttrDefGenerator generator(records, os);
+                  return generator.emitList(attrDialect);
+                });
 //===----------------------------------------------------------------------===//
 // TypeDef
 

@andykaylor
Copy link
Contributor Author

This change (llvm/clangir#1318) in the clangir incubator repo shows the intended use of the generated files.

@River707
Copy link
Contributor

River707 commented Feb 8, 2025

It's not clear to me from reading the clangir pr that the existing attrdef list generation can't be used in combination with TypeSwitch and templates to get what you want. I dont think we need anything additional to be generated.

Have you tried to use the existing GET_ATTRDEF_LIST logic for this? You would need to have overloaded 'visit' methods (i.e. can't have the name of the attribute in the visit method), but this should all just work.

Copy link
Contributor

@River707 River707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking RC

@andykaylor
Copy link
Contributor Author

@River707 Thanks! I had tried to figure out a way to do this with the existing GET_ATTRDEF_LIST but I couldn't find my way to a solution. It looks like TypeSwitch is the piece I was missing.

I did still run into a problem trying to adapt my visitor base class, because (at least with the compiler I'm using) TypeSwitch doesn't like taking a template parameter for the return type. That is, when I try this:

  RetTy visit(mlir::Attribute attr) {
    #define GET_ATTRDEF_LIST
    return llvm::TypeSwitch<mlir::Attribute, RetTy>(attr)
    .Case<
#include "clang/CIR/Dialect/IR/CIROpsAttributes.cpp.inc"
    >([&](auto attrT) { return getImpl()->visitCirAttr(attrT); })
      .Default([&](auto attrT) { return getImpl()->visitNonCirAttr(attrT); });
  }

I get a bunch of errors like this:

llvm-project/build/tools/clang/include/clang/CIR/Dialect/IR/CIROpsAttributes.cpp.inc: In member function ‘RetTy cir::CirAttrVisitor<ImplClass, RetTy>::visit(mlir::Attribute)’:
llvm-project/build/tools/clang/include/clang/CIR/Dialect/IR/CIROpsAttributes.cpp.inc:12:16: error: expected primary-expression before ‘,’ token
   12 | ::cir::LangAttr,
      |                ^

But if I switch to this:

    return llvm::TypeSwitch<mlir::Attribute, mlir::Value>(attr)

It works. That feels like a compiler bug, but I could be missing something.

However, I don't think I need it to work. I updated my clangir patch to just use TypeSwitch in my visitor implementation class, and I don't think I will even need to use GET_ATTRDEF_LIST.

If you're interested, the updated clangir change is here: llvm/clangir#1330

I'll close this PR if the new clangir implementation is acceptable to the reviewers there.

@River707
Copy link
Contributor

@River707 Thanks! I had tried to figure out a way to do this with the existing GET_ATTRDEF_LIST but I couldn't find my way to a solution. It looks like TypeSwitch is the piece I was missing.

I did still run into a problem trying to adapt my visitor base class, because (at least with the compiler I'm using) TypeSwitch doesn't like taking a template parameter for the return type. That is, when I try this:

  RetTy visit(mlir::Attribute attr) {
    #define GET_ATTRDEF_LIST
    return llvm::TypeSwitch<mlir::Attribute, RetTy>(attr)
    .Case<
#include "clang/CIR/Dialect/IR/CIROpsAttributes.cpp.inc"
    >([&](auto attrT) { return getImpl()->visitCirAttr(attrT); })
      .Default([&](auto attrT) { return getImpl()->visitNonCirAttr(attrT); });
  }

I get a bunch of errors like this:

llvm-project/build/tools/clang/include/clang/CIR/Dialect/IR/CIROpsAttributes.cpp.inc: In member function ‘RetTy cir::CirAttrVisitor<ImplClass, RetTy>::visit(mlir::Attribute)’:
llvm-project/build/tools/clang/include/clang/CIR/Dialect/IR/CIROpsAttributes.cpp.inc:12:16: error: expected primary-expression before ‘,’ token
   12 | ::cir::LangAttr,
      |                ^

But if I switch to this:

    return llvm::TypeSwitch<mlir::Attribute, mlir::Value>(attr)

It works. That feels like a compiler bug, but I could be missing something.

However, I don't think I need it to work. I updated my clangir patch to just use TypeSwitch in my visitor implementation class, and I don't think I will even need to use GET_ATTRDEF_LIST.

If you're interested, the updated clangir change is here: llvm/clangir#1330

I'll close this PR if the new clangir implementation is acceptable to the reviewers there.

Nice, I'm glad that worked. This use case is one of the reasons I wrote TypeSwitch initially. The template error seems a bit weird, I feel like I've written that exact code before.

@andykaylor
Copy link
Contributor Author

Withdrawn in favor of TypeSwitch-based implementation

@andykaylor andykaylor closed this Feb 11, 2025
andykaylor added a commit to andykaylor/llvm-project that referenced this pull request Feb 28, 2025
We previously discussed having an mlir-tblgen utility to complete the
CIRAttrVisitor implementation with all support attribute types, but
when I proposed an implementation to do this, a reviewer suggested
using TypeSwitch instead, and I have done that in the incubator.

See llvm#126332

This change brings the TypeSwitch implementation into the upstream repo
to replace the visitor class.
andykaylor added a commit that referenced this pull request Feb 28, 2025
The previously upstreamed lowering from ClangIR to LLVM IR diverged from
the incubator implementation, but when the incubator was updated to
incorporate these changes some issues arose which require the upstream
implementation to be modified to re-align with the incubator.

First, in the earlier upstream implementation a CIRAttrVisitor class was
introduced with the intention that an mlir-tblgen based extension would
be created to automatically add all CIR attributes to the visitor. When
I proposed this in mlir-tblgen a reviewer suggested that what I wanted
could be better accomplished with TypeSwitch.

See #126332

This was done in the incubator, and here I am bringing that
implementation upstream.

The other issue was that the global op initialization in the incubator
had more cases than I had accounted for in my previous upstream
refactoring. I did still refactor the incubator code, but not in quite
the same way as the upstream code. This change re-aligns the two.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Feb 28, 2025
…(#129293)

The previously upstreamed lowering from ClangIR to LLVM IR diverged from
the incubator implementation, but when the incubator was updated to
incorporate these changes some issues arose which require the upstream
implementation to be modified to re-align with the incubator.

First, in the earlier upstream implementation a CIRAttrVisitor class was
introduced with the intention that an mlir-tblgen based extension would
be created to automatically add all CIR attributes to the visitor. When
I proposed this in mlir-tblgen a reviewer suggested that what I wanted
could be better accomplished with TypeSwitch.

See llvm/llvm-project#126332

This was done in the incubator, and here I am bringing that
implementation upstream.

The other issue was that the global op initialization in the incubator
had more cases than I had accounted for in my previous upstream
refactoring. I did still refactor the incubator code, but not in quite
the same way as the upstream code. This change re-aligns the two.
@andykaylor andykaylor deleted the tablegen-attrdef-list branch March 4, 2025 22:53
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
The previously upstreamed lowering from ClangIR to LLVM IR diverged from
the incubator implementation, but when the incubator was updated to
incorporate these changes some issues arose which require the upstream
implementation to be modified to re-align with the incubator.

First, in the earlier upstream implementation a CIRAttrVisitor class was
introduced with the intention that an mlir-tblgen based extension would
be created to automatically add all CIR attributes to the visitor. When
I proposed this in mlir-tblgen a reviewer suggested that what I wanted
could be better accomplished with TypeSwitch.

See llvm#126332

This was done in the incubator, and here I am bringing that
implementation upstream.

The other issue was that the global op initialization in the incubator
had more cases than I had accounted for in my previous upstream
refactoring. I did still refactor the incubator code, but not in quite
the same way as the upstream code. This change re-aligns the two.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants