[mlir] Decouple enum generation from attributes, adding EnumInfo and EnumCase #132148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
mlir:core
|
@llvm/pr-subscribers-mlir-ods @llvm/pr-subscribers-mlir Author: Krzysztof Drewniak (krzysz00) ChangesThis commit pulls apart the inherent attribute dependence of classes like EnumAttrInfo and EnumAttrCase, factoring them out into simpler EnumCase and EnumInfo variants. This allows specifying the cases of an enum without needing to make the cases, or the EnumInfo itself, a subclass of SignlessIntegerAttrBase. The existing classes are retained as subclasses of the new ones, both for backwards compatibility and to allow attribute-specific information. In addition, the new BitEnum class changes its default printer/parser behavior: cases when multiple keywords appear, like having both nuw and nsw in overflow flags, will no longer be quoted by the operator<<, and the FieldParser instance will now expect multiple keywords. All instances of BitEnumAttr retain the old behavior. Finally, this commit improves the (Sadly, bitcode auto-upgrade is hampered by the lack of the ability to optionally parse an attribute.) Patch is 131.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/132148.diff 28 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td
index a9de787806452..34a30a00790ea 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td
@@ -485,17 +485,16 @@ def DISubprogramFlags : I32BitEnumAttr<
// IntegerOverflowFlags
//===----------------------------------------------------------------------===//
-def IOFnone : I32BitEnumAttrCaseNone<"none">;
-def IOFnsw : I32BitEnumAttrCaseBit<"nsw", 0>;
-def IOFnuw : I32BitEnumAttrCaseBit<"nuw", 1>;
+def IOFnone : I32BitEnumCaseNone<"none">;
+def IOFnsw : I32BitEnumCaseBit<"nsw", 0>;
+def IOFnuw : I32BitEnumCaseBit<"nuw", 1>;
-def IntegerOverflowFlags : I32BitEnumAttr<
+def IntegerOverflowFlags : I32BitEnum<
"IntegerOverflowFlags",
"LLVM integer overflow flags",
[IOFnone, IOFnsw, IOFnuw]> {
let separator = ", ";
let cppNamespace = "::mlir::LLVM";
- let genSpecializedAttr = 0;
let printBitEnumPrimaryGroups = 1;
}
@@ -504,6 +503,11 @@ def LLVM_IntegerOverflowFlagsAttr :
let assemblyFormat = "`<` $value `>`";
}
+def LLVM_IntegerOverflowFlagsProp :
+ NamedEnumPropWithAttrForm<IntegerOverflowFlags, "overflow", LLVM_IntegerOverflowFlagsAttr> {
+ let defaultValue = enum.cppType # "::" # "none";
+}
+
//===----------------------------------------------------------------------===//
// FastmathFlags
//===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 90cc851c0a3b2..75f23e5b46c5f 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -60,7 +60,7 @@ class LLVM_IntArithmeticOpWithOverflowFlag<string mnemonic, string instName,
list<Trait> traits = []> :
LLVM_ArithmeticOpBase<AnySignlessInteger, mnemonic, instName,
!listconcat([DeclareOpInterfaceMethods<IntegerOverflowFlagsInterface>], traits)> {
- dag iofArg = (ins EnumProp<"IntegerOverflowFlags", "", "IntegerOverflowFlags::none">:$overflowFlags);
+ dag iofArg = (ins LLVM_IntegerOverflowFlagsProp:$overflowFlags);
let arguments = !con(commonArgs, iofArg);
string mlirBuilder = [{
@@ -69,7 +69,7 @@ class LLVM_IntArithmeticOpWithOverflowFlag<string mnemonic, string instName,
$res = op;
}];
let assemblyFormat = [{
- $lhs `,` $rhs `` custom<OverflowFlags>($overflowFlags) attr-dict `:` type($res)
+ $lhs `,` $rhs ($overflowFlags^)? attr-dict `:` type($res)
}];
string llvmBuilder =
"$res = builder.Create" # instName #
@@ -563,10 +563,10 @@ class LLVM_CastOpWithOverflowFlag<string mnemonic, string instName, Type type,
Type resultType, list<Trait> traits = []> :
LLVM_Op<mnemonic, !listconcat([Pure], [DeclareOpInterfaceMethods<IntegerOverflowFlagsInterface>], traits)>,
LLVM_Builder<"$res = builder.Create" # instName # "($arg, $_resultType, /*Name=*/\"\", op.hasNoUnsignedWrap(), op.hasNoSignedWrap());"> {
- let arguments = (ins type:$arg, EnumProp<"IntegerOverflowFlags", "", "IntegerOverflowFlags::none">:$overflowFlags);
+ let arguments = (ins type:$arg, LLVM_IntegerOverflowFlagsProp:$overflowFlags);
let results = (outs resultType:$res);
let builders = [LLVM_OneResultOpBuilder];
- let assemblyFormat = "$arg `` custom<OverflowFlags>($overflowFlags) attr-dict `:` type($arg) `to` type($res)";
+ let assemblyFormat = "$arg ($overflowFlags^)? attr-dict `:` type($arg) `to` type($res)";
string llvmInstName = instName;
string mlirBuilder = [{
auto op = $_builder.create<$_qualCppClassName>(
diff --git a/mlir/include/mlir/IR/EnumAttr.td b/mlir/include/mlir/IR/EnumAttr.td
index 9fec28f03ec28..aedda1d952eb6 100644
--- a/mlir/include/mlir/IR/EnumAttr.td
+++ b/mlir/include/mlir/IR/EnumAttr.td
@@ -10,12 +10,13 @@
#define ENUMATTR_TD
include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/Properties.td"
//===----------------------------------------------------------------------===//
// Enum attribute kinds
-// Additional information for an enum attribute case.
-class EnumAttrCaseInfo<string sym, int intVal, string strVal> {
+// Additional information for an enum case.
+class EnumCase<string sym, int intVal, string strVal, int widthVal> {
// The C++ enumerant symbol.
string symbol = sym;
@@ -26,29 +27,56 @@ class EnumAttrCaseInfo<string sym, int intVal, string strVal> {
// The string representation of the enumerant. May be the same as symbol.
string str = strVal;
+
+ // The bitwidth of the enum.
+ int width = widthVal;
}
// An enum attribute case stored with IntegerAttr, which has an integer value,
// its representation as a string and a C++ symbol name which may be different.
+// Not needed when using the newer `EnumCase` form for defining enum cases.
class IntEnumAttrCaseBase<I intType, string sym, string strVal, int intVal> :
- EnumAttrCaseInfo<sym, intVal, strVal>,
+ EnumCase<sym, intVal, strVal, intType.bitwidth>,
SignlessIntegerAttrBase<intType, "case " # strVal> {
let predicate =
CPred<"::llvm::cast<::mlir::IntegerAttr>($_self).getInt() == " # intVal>;
}
-// Cases of integer enum attributes with a specific type. By default, the string
+// Cases of integer enums with a specific type. By default, the string
// representation is the same as the C++ symbol name.
+class I32EnumCase<string sym, int val, string str = sym>
+ : EnumCase<sym, val, str, 32>;
+class I64EnumCase<string sym, int val, string str = sym>
+ : EnumCase<sym, val, str, 64>;
+
+// Cases of integer enum attributes with a specific type. By default, the string
+// representation is the same as the C++ symbol name. These forms
+// are not needed when using the newer `EnumCase` form.
class I32EnumAttrCase<string sym, int val, string str = sym>
: IntEnumAttrCaseBase<I32, sym, str, val>;
class I64EnumAttrCase<string sym, int val, string str = sym>
: IntEnumAttrCaseBase<I64, sym, str, val>;
-// A bit enum case stored with an IntegerAttr. `val` here is *not* the ordinal
-// number of a bit that is set. It is an integer value with bits set to match
-// the case.
+// A bit enum case. `val` here is *not* the ordinal number of a bit
+// that is set. It is an integer value with bits set to match the case.
+class BitEnumCaseBase<string sym, int val, string str, int width> :
+ EnumCase<sym, val, str, width>;
+// Bit enum attr cases. The string representation is the same as the C++ symbol
+// name unless otherwise specified.
+class I8BitEnumCase<string sym, int val, string str = sym>
+ : BitEnumCaseBase<sym, val, str, 8>;
+class I16BitEnumCase<string sym, int val, string str = sym>
+ : BitEnumCaseBase<sym, val, str, 16>;
+class I32BitEnumCase<string sym, int val, string str = sym>
+ : BitEnumCaseBase<sym, val, str, 32>;
+class I64BitEnumCase<string sym, int val, string str = sym>
+ : BitEnumCaseBase<sym, val, str, 64>;
+
+// A form of `BitEnumCaseBase` that also inherits from `Attr` and encodes
+// the width of the enum, which was defined when enums were always
+// stored in attributes.
class BitEnumAttrCaseBase<I intType, string sym, int val, string str = sym> :
- EnumAttrCaseInfo<sym, val, str>,
+ BitEnumCaseBase<sym, val, str, intType.bitwidth>,
SignlessIntegerAttrBase<intType, "case " #str>;
class I8BitEnumAttrCase<string sym, int val, string str = sym>
@@ -61,6 +89,19 @@ class I64BitEnumAttrCase<string sym, int val, string str = sym>
: BitEnumAttrCaseBase<I64, sym, val, str>;
// The special bit enum case with no bits set (i.e. value = 0).
+class BitEnumCaseNone<string sym, string str, int width>
+ : BitEnumCaseBase<sym, 0, str, width>;
+
+class I8BitEnumCaseNone<string sym, string str = sym>
+ : BitEnumCaseNone<sym, str, 8>;
+class I16BitEnumCaseNone<string sym, string str = sym>
+ : BitEnumCaseNone<sym, str, 16>;
+class I32BitEnumCaseNone<string sym, string str = sym>
+ : BitEnumCaseNone<sym, str, 32>;
+class I64BitEnumCaseNone<string sym, string str = sym>
+ : BitEnumCaseNone<sym, str, 64>;
+
+// Older forms, used when enums were necessarily attributes.
class I8BitEnumAttrCaseNone<string sym, string str = sym>
: I8BitEnumAttrCase<sym, 0, str>;
class I16BitEnumAttrCaseNone<string sym, string str = sym>
@@ -70,6 +111,24 @@ class I32BitEnumAttrCaseNone<string sym, string str = sym>
class I64BitEnumAttrCaseNone<string sym, string str = sym>
: I64BitEnumAttrCase<sym, 0, str>;
+// A bit enum case for a single bit, specified by a bit position `pos`.
+// The `pos` argument refers to the index of the bit, and is limited
+// to be in the range [0, width).
+class BitEnumCaseBit<string sym, int pos, string str, int width>
+ : BitEnumCaseBase<sym, !shl(1, pos), str, width> {
+ assert !and(!ge(pos, 0), !lt(pos, width)),
+ "bit position larger than underlying storage";
+}
+
+class I8BitEnumCaseBit<string sym, int pos, string str = sym>
+ : BitEnumCaseBit<sym, pos, str, 8>;
+class I16BitEnumCaseBit<string sym, int pos, string str = sym>
+ : BitEnumCaseBit<sym, pos, str, 16>;
+class I32BitEnumCaseBit<string sym, int pos, string str = sym>
+ : BitEnumCaseBit<sym, pos, str, 32>;
+class I64BitEnumCaseBit<string sym, int pos, string str = sym>
+ : BitEnumCaseBit<sym, pos, str, 64>;
+
// A bit enum case for a single bit, specified by a bit position.
// The pos argument refers to the index of the bit, and is limited
// to be in the range [0, bitwidth).
@@ -90,12 +149,17 @@ class I64BitEnumAttrCaseBit<string sym, int pos, string str = sym>
// A bit enum case for a group/list of previously declared cases, providing
// a convenient alias for that group.
+class BitEnumCaseGroup<string sym, list<BitEnumCaseBase> cases, string str = sym>
+ : BitEnumCaseBase<sym,
+ !foldl(0, cases, value, bitcase, !or(value, bitcase.value)),
+ str, !head(cases).width>;
+
+// The attribute-only form of `BitEnumCaseGroup`.
class BitEnumAttrCaseGroup<I intType, string sym,
- list<BitEnumAttrCaseBase> cases, string str = sym>
+ list<BitEnumCaseBase> cases, string str = sym>
: BitEnumAttrCaseBase<intType, sym,
!foldl(0, cases, value, bitcase, !or(value, bitcase.value)),
str>;
-
class I8BitEnumAttrCaseGroup<string sym, list<BitEnumAttrCaseBase> cases,
string str = sym>
: BitEnumAttrCaseGroup<I8, sym, cases, str>;
@@ -109,29 +173,36 @@ class I64BitEnumAttrCaseGroup<string sym, list<BitEnumAttrCaseBase> cases,
string str = sym>
: BitEnumAttrCaseGroup<I64, sym, cases, str>;
-// Additional information for an enum attribute.
-class EnumAttrInfo<
- string name, list<EnumAttrCaseInfo> cases, Attr baseClass> :
- Attr<baseClass.predicate, baseClass.summary> {
-
+// Information describing an enum and the functions that should be generated for it.
+class EnumInfo<string name, string summaryValue, list<EnumCase> cases, int width> {
+ string summary = summaryValue;
// Generate a description of this enums members for the MLIR docs.
- let description =
+ string description =
"Enum cases:\n" # !interleave(
!foreach(case, cases,
"* " # case.str # " (`" # case.symbol # "`)"), "\n");
+ // The C++ namespace for this enum
+ string cppNamespace = "";
+
// The C++ enum class name
string className = name;
+ // C++ type wrapped by attribute
+ string cppType = cppNamespace # "::" # className;
+
// List of all accepted cases
- list<EnumAttrCaseInfo> enumerants = cases;
+ list<EnumCase> enumerants = cases;
// The following fields are only used by the EnumsGen backend to generate
// an enum class definition and conversion utility functions.
+ // The bitwidth underlying the class
+ int bitwidth = width;
+
// The underlying type for the C++ enum class. An empty string mean the
// underlying type is not explicitly specified.
- string underlyingType = "";
+ string underlyingType = "uint" # width # "_t";
// The name of the utility function that converts a value of the underlying
// type to the corresponding symbol. It will have the following signature:
@@ -165,6 +236,15 @@ class EnumAttrInfo<
// static constexpr unsigned <fn-name>();
// ```
string maxEnumValFnName = "getMaxEnumValFor" # name;
+}
+
+// A wrapper around `EnumInfo` that also makes the Enum an attribute
+// if `genSeecializedAttr` is 1 (though `EnumAttr` is the preferred means
+// to accomplish this) or declares that the enum will be stored in an attribute.
+class EnumAttrInfo<
+ string name, list<EnumCase> cases, SignlessIntegerAttrBase baseClass> :
+ EnumInfo<name, baseClass.summary, cases, !cast<I>(baseClass.valueType).bitwidth>,
+ Attr<baseClass.predicate, baseClass.summary> {
// Generate specialized Attribute class
bit genSpecializedAttr = 1;
@@ -188,15 +268,25 @@ class EnumAttrInfo<
baseAttrClass.constBuilderCall);
let valueType = baseAttrClass.valueType;
- // C++ type wrapped by attribute
- string cppType = cppNamespace # "::" # className;
-
// Parser and printer code used by the EnumParameter class, to be provided by
// derived classes
string parameterParser = ?;
string parameterPrinter = ?;
}
+// An attribute holding a single integer value.
+class IntEnum<string name, string summary, list<EnumCase> cases, int width>
+ : EnumInfo<name,
+ !if(!empty(summary), "allowed i" # width # " cases: " #
+ !interleave(!foreach(case, cases, case.value), ", "),
+ summary),
+ cases, width>;
+
+class I32Enum<string name, string summary, list<EnumCase> cases>
+ : IntEnum<name, summary, cases, 32>;
+class I64Enum<string name, string summary, list<EnumCase> cases>
+ : IntEnum<name, summary, cases, 32>;
+
// An enum attribute backed by IntegerAttr.
//
// Op attributes of this kind are stored as IntegerAttr. Extra verification will
@@ -245,13 +335,73 @@ class I64EnumAttr<string name, string summary, list<I64EnumAttrCase> cases> :
let underlyingType = "uint64_t";
}
+// The base mixin for bit enums that are stored as an integer.
+// This is used by both BitEnum and BitEnumAttr, which need to have a set of
+// extra properties that bit enums have which normal enums don't. However,
+// we can't just use BitEnum as a base class of BitEnumAttr, since BitEnumAttr
+// also inherits from EnumAttrInfo, causing double inheritance of EnumInfo.
+class BitEnumBase<list<BitEnumCaseBase> cases> {
+ // Determine "valid" bits from enum cases for error checking
+ int validBits = !foldl(0, cases, value, bitcase, !or(value, bitcase.value));
+
+ // The delimiter used to separate bit enum cases in strings. Only "|" and
+ // "," (along with optional spaces) are supported due to the use of the
+ // parseSeparatorFn in parameterParser below.
+ // Spaces in the separator string are used for printing, but will be optional
+ // for parsing.
+ string separator = "|";
+ assert !or(!ge(!find(separator, "|"), 0), !ge(!find(separator, ","), 0)),
+ "separator must contain '|' or ',' for parameter parsing";
+
+ // Print the "primary group" only for bits that are members of case groups
+ // that have all bits present. When the value is 0, printing will display both
+ // both individual bit case names AND the names for all groups that the bit is
+ // contained in. When the value is 1, for each bit that is set AND is a member
+ // of a group with all bits set, only the "primary group" (i.e. the first
+ // group with all bits set in reverse declaration order) will be printed (for
+ // conciseness).
+ bit printBitEnumPrimaryGroups = 0;
+
+ // 1 if the operator<< for this enum should put quotes around values with
+ // multiple entries. Off by default in the general case but on for BitEnumAttrs
+ // since that was the original behavior.
+ bit printBitEnumQuoted = 0;
+}
+
+// A bit enum stored as an integer.
+//
+// Enums of these kind are staored as an integer. Attributes or properties deriving
+// from this enum will have additional verification generated on them to make sure
+// only allowed bits are set. Helper methods are generated to parse a sring of enum
+// values generated by the specified separator to a symbol and vice versa.
+class BitEnum<string name, string summary, list<BitEnumCaseBase> cases, int width>
+ : EnumInfo<name, summary, cases, width>, BitEnumBase<cases> {
+ // We need to return a string because we may concatenate symbols for multiple
+ // bits together.
+ let symbolToStringFnRetType = "std::string";
+}
+
+class I8BitEnum<string name, string summary,
+ list<BitEnumCaseBase> cases>
+ : BitEnum<name, summary, cases, 8>;
+class I16BitEnum<string name, string summary,
+ list<BitEnumCaseBase> cases>
+ : BitEnum<name, summary, cases, 16>;
+class I32BitEnum<string name, string summary,
+ list<BitEnumCaseBase> cases>
+ : BitEnum<name, summary, cases, 32>;
+
+class I64BitEnum<string name, string summary,
+ list<BitEnumCaseBase> cases>
+ : BitEnum<name, summary, cases, 64>;
+
// A bit enum stored with an IntegerAttr.
//
// Op attributes of this kind are stored as IntegerAttr. Extra verification will
// be generated on the integer to make sure only allowed bits are set. Besides,
// helper methods are generated to parse a string separated with a specified
// delimiter to a symbol and vice versa.
-class BitEnumAttrBase<I intType, list<BitEnumAttrCaseBase> cases,
+class BitEnumAttrBase<I intType, list<BitEnumCaseBase> cases,
string summary>
: SignlessIntegerAttrBase<intType, summary> {
let predicate = And<[
@@ -264,24 +414,13 @@ class BitEnumAttrBase<I intType, list<BitEnumAttrCaseBase> cases,
}
class BitEnumAttr<I intType, string name, string summary,
- list<BitEnumAttrCaseBase> cases>
- : EnumAttrInfo<name, cases, BitEnumAttrBase<intType, cases, summary>> {
- // Determine "valid" bits from enum cases for error checking
- int validBits = !foldl(0, cases, value, bitcase, !or(value, bitcase.value));
-
+ list<BitEnumCaseBase> cases>
+ : EnumAttrInfo<name, cases, BitEnumAttrBase<intType, cases, summary>>,
+ BitEnumBase<cases> {
// We need to return a string because we may concatenate symbols for multiple
// bits together.
let symbolToStringFnRetType = "std::string";
- // The delimiter used to separate bit enum cases in strings. Only "|" and
- // "," (along with optional spaces) are supported due to the use of the
- // parseSeparatorFn in parameterParser below.
- // Spaces in the separator string are used for printing, but will be optional
- // for parsing.
- string separator = "|";
- assert !or(!ge(!find(separator, "|"), 0), !ge(!find(separator, ","), 0)),
- "separator must contain '|' or ',' for parameter parsing";
-
// Parsing function that corresponds to the enum separator. Only
// "," and "|" are supported by this definition.
string parseSeparatorFn = !if(!ge(!find(separator, "|"), 0),
@@ -312,36 +451,30 @@ class BitEnumAttr<I intType, string name, string summary,
// Print the enum by calling `symbolToString`.
let parameterPrinter = "$_printer << " # symbolToStringFnName # "($_self)";
- // Print the "primary group" only for bits that are members of case groups
- // that have all bits present. When the value is 0, printing will display both
- // both individual bit case names AND the names for all groups that the bit is
- // contained in. When the value is 1, for each bit that is set AND is a member
- // of a group with all bits set, only the "primary group" (i.e. the first
- // group with all bits set in reverse declaration order) will be printed (for
- // conciseness).
- bit printBitEnumPrimaryGroups = 0;
+ // Use old-style operator<< and FieldParser for compatibility
+ let printBitEnumQuoted = 1;
}
class I8BitEnumAttr<string name, string summary,
- list<BitEnumAttrCaseBase> cases>
+ list<BitEnumCaseBase> cases>
: BitEnumAttr<I8, name, summary, cases> {
let underlyingType = "uint8_t";
}
class I16BitEnumAttr<string name, string summary,
- list<BitEnumAttrCaseBase> cases>
+ list<BitEnumCaseBase> cases>
...
[truncated]
|
|
@matthias-springer I can split the EnumProp changes into their own PR, not I don't think there split points other than that |
krzysz00
changed the title
krzysz00
force-pushed
the
users/krzysz00/refactor-enum-enum-attr
branch
from
be26be6 to
c8b6e56
Compare
|
Have split the EnumProp thing into #132349, hopefully this improves reviewability |
|
I'm not super familiar with this part of the code base. |
| struct FieldParser; | ||
|
|
||
| template<> | ||
| struct FieldParser<{0}, {0}> {{ |
There was a problem hiding this comment.
How was this handled before? I expected that this is would replace another FieldParser implementation.
There was a problem hiding this comment.
There's a FieldParser implementation above that's the old-style bit enum. It's handled by always quoting bit enum values that have more than one bit set.
Usually, these FieldParser values were very much unused, as there are separate parser implementatinos both in EnumParameter and in other bits of mlir-tblgen that mean this code was basically never called.
|
Thx for breaking up the PR. It's still quite large, maybe this could also be a separate one: Or maybe the part that moves code to In my experience, anything that's larger than 100 loc is taking a quite long time to get reviewed... |
krzysz00
added a commit
that referenced
this pull request
Mar 24, 2025
This moves the EnumAttrCase and EnumAttr classes from Attribute.h/.cpp to a new EnumInfo.h/cpp and renames them to EnumCase and EnumInfo, respectively. This doesn't change any of the tablegen files or any user-facing aspects of the enum attribute generation system, just reorganizes code in order to make main PR (#132148) shorter.
krzysz00
force-pushed
the
users/krzysz00/refactor-enum-enum-attr
branch
from
c8b6e56 to
509fe55
Compare
krzysz00
added a commit
that referenced
this pull request
Mar 27, 2025
This moves the EnumAttrCase and EnumAttr classes from Attribute.h/.cpp to a new EnumInfo.h/cpp and renames them to EnumCase and EnumInfo, respectively. This doesn't change any of the tablegen files or any user-facing aspects of the enum attribute generation system, just reorganizes code in order to make main PR (#132148) shorter.
…EnumCase This commit pulls apart the inherent attribute dependence of classes like EnumAttrInfo and EnumAttrCase, factoring them out into simpler EnumCase and EnumInfo variants. This allows specifying the cases of an enum without needing to make the cases, or the EnumInfo itself, a subclass of SignlessIntegerAttrBase. The existing classes are retained as subclasses of the new ones, both for backwards compatibility and to allow attribute-specific information. In addition, the new BitEnum class changes its default printer/parser behavior: cases when multiple keywords appear, like having both nuw and nsw in overflow flags, will no longer be quoted by the operator<<, and the FieldParser instance will now expect multiple keywords. All instances of BitEnumAttr retain the old behavior.
krzysz00
force-pushed
the
users/krzysz00/refactor-enum-enum-attr
branch
from
509fe55 to
57a1e08
Compare
|
Have updated documentation. Do folks have a strong opinion on if I should move the new parser stuff into a separate commit? |
matthias-springer
approved these changes
Mar 27, 2025
|
@krzysz00 Does this affect any of the enum binding code in IREE? |
nah I doubt it - the handwritten bindings in compiler/bindings/python/IREECompilerDialectsModule.cpp are ... handwritten (so unaffected). Besides that I don't think (don't remember) IREE (or tuner...) is using these autogenerated bindings ( |
|
Re IREE bindings: If nothing else, this change should be NFC for anything that uses Whether the Python bindings for non-attribute enums are good / how Python binds non-attribute properties ... I'll cross that bridge when I get there |
It doesn't - the lines you touched (there-abouts) are the only place where Attribute bindings are automatically emitted (and "handwritten" bindings should not be affected in any way). |
krzysz00
added a commit
that referenced
this pull request
Apr 14, 2025
This commit improves the `EnumProp` class, causing it to wrap around an `EnumInfo` just like` EnumAttr` does. This EnumProp also has logic for converting to/from an integer attribute and for being read and written as bitcode. The following variants of `EnumProp` are provided: - `EnumPropWithAttrForm` - an EnumProp that can be constructed from (and will be converted to, if `storeInCustomAttribute` is true) a custom attribute, like an `EnumAttr`, instead of a plain integer. This is meant for backwards compatibility with code that uses enum attributes. `NamedEnumProp` adds a "`mnemonic` `<` $enum `>`" syntax around the enum, replicating a common pattern seen in MLIR printers and allowing for reduced ambiguity. `NamedEnumPropWithAttrForm` combines both of these extensions. (Sadly, bytecode auto-upgrade is hampered by the lack of the ability to optionally parse an attribute.) Depends on #132148
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
This commit improves the `EnumProp` class, causing it to wrap around an `EnumInfo` just like` EnumAttr` does. This EnumProp also has logic for converting to/from an integer attribute and for being read and written as bitcode. The following variants of `EnumProp` are provided: - `EnumPropWithAttrForm` - an EnumProp that can be constructed from (and will be converted to, if `storeInCustomAttribute` is true) a custom attribute, like an `EnumAttr`, instead of a plain integer. This is meant for backwards compatibility with code that uses enum attributes. `NamedEnumProp` adds a "`mnemonic` `<` $enum `>`" syntax around the enum, replicating a common pattern seen in MLIR printers and allowing for reduced ambiguity. `NamedEnumPropWithAttrForm` combines both of these extensions. (Sadly, bytecode auto-upgrade is hampered by the lack of the ability to optionally parse an attribute.) Depends on llvm#132148
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit pulls apart the inherent attribute dependence of classes like EnumAttrInfo and EnumAttrCase, factoring them out into simpler EnumCase and EnumInfo variants. This allows specifying the cases of an enum without needing to make the cases, or the EnumInfo itself, a subclass of SignlessIntegerAttrBase.
The existing classes are retained as subclasses of the new ones, both for backwards compatibility and to allow attribute-specific information.
In addition, the new BitEnum class changes its default printer/parser behavior: cases when multiple keywords appear, like having both nuw and nsw in overflow flags, will no longer be quoted by the operator<<, and the FieldParser instance will now expect multiple keywords. All instances of BitEnumAttr retain the old behavior.