[Mlir-commits] [mlir] [mlir][ODS] Allow a space-only separator for bit enums (PR #237149)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Fri Oct 30 06:52:17 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-ods
Author: Henrich Lauko (xlauko)
<details>
<summary>Changes</summary>
Bit enums required a separator containing `|` or `,`, so a flag set could not be written as bare keywords like LLVM IR's `nuw nsw`, and dialects wanting that syntax (e.g. ClangIR overflow flags) had to hand-write an `EnumProp` parser and printer. This allows `separator = " "` on new-style `BitEnum`s: the generated `FieldParser` reads case keywords until the next token is not one, symbolize splits on spaces, and the op-format `enum` directive parses the same way, while a case literal placed right after such an enum is rejected as ambiguous. Legacy `BitEnumAttr` still requires `|` or `,`. Tests cover the generated parser, property/attribute/`enum`-directive roundtrips, and the new format error.
Assisted-by: Claude Code (Claude Opus 5.5).
---
Full diff: https://github.com/llvm/llvm-project/pull/237149.diff
10 Files Affected:
- (modified) mlir/include/mlir/IR/EnumAttr.td (+9-7)
- (modified) mlir/test/IR/attribute.mlir (+23)
- (modified) mlir/test/IR/enum-attr-roundtrip.mlir (+24)
- (modified) mlir/test/lib/Dialect/Test/TestEnumDefs.td (+12)
- (modified) mlir/test/lib/Dialect/Test/TestOps.td (+15)
- (modified) mlir/test/lib/Dialect/Test/TestOpsSyntax.td (+10)
- (modified) mlir/test/mlir-tblgen/enums-gen.td (+20)
- (modified) mlir/test/mlir-tblgen/op-format-invalid.td (+14)
- (modified) mlir/tools/mlir-tblgen/EnumsGen.cpp (+38-2)
- (modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+33-11)
``````````diff
diff --git a/mlir/include/mlir/IR/EnumAttr.td b/mlir/include/mlir/IR/EnumAttr.td
index 1c3a223dbb8496..4841798464d0e1 100644
--- a/mlir/include/mlir/IR/EnumAttr.td
+++ b/mlir/include/mlir/IR/EnumAttr.td
@@ -362,14 +362,14 @@ 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.
+ // The delimiter used to separate bit enum cases in strings. It must contain
+ // "|" or ",", or consist of spaces only. Spaces around "|" or "," are used
+ // for printing but are optional for parsing. With a space-only separator,
+ // the cases are parsed as a sequence of keywords, e.g. `nuw nsw`.
string separator = "|";
- assert !or(!ge(!find(separator, "|"), 0), !ge(!find(separator, ","), 0)),
- "separator must contain '|' or ',' for parameter parsing";
+ assert !or(!ge(!find(separator, "|"), 0), !ge(!find(separator, ","), 0),
+ !and(!ne(separator, ""), !eq(!subst(" ", "", separator), ""))),
+ "separator must contain '|' or ',', or be spaces only";
// 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
@@ -441,6 +441,8 @@ class BitEnumAttr<I intType, string name, string summary,
// Parsing function that corresponds to the enum separator. Only
// "," and "|" are supported by this definition.
+ assert !or(!ge(!find(separator, "|"), 0), !ge(!find(separator, ","), 0)),
+ "BitEnumAttr separator must contain '|' or ','";
string parseSeparatorFn = !if(!ge(!find(separator, "|"), 0),
"parseOptionalVerticalBar",
"parseOptionalComma");
diff --git a/mlir/test/IR/attribute.mlir b/mlir/test/IR/attribute.mlir
index ea378439c0f3bb..a931eabd695709 100644
--- a/mlir/test/IR/attribute.mlir
+++ b/mlir/test/IR/attribute.mlir
@@ -542,6 +542,29 @@ func.func @allowed_cases_pass() {
// -----
+// CHECK-LABEL: func @space_separated_cases_pass
+func.func @space_separated_cases_pass() {
+ // CHECK: test.op_with_bit_enum_space <user group>
+ "test.op_with_bit_enum_space"() {
+ value = #test.bit_enum_space<group user>
+ } : () -> ()
+ // CHECK: test.op_with_bit_enum_space <user group other> tag 1 : i32
+ test.op_with_bit_enum_space <other user group> tag 1 : i32
+ // CHECK: test.op_with_bit_enum_space_directive user other tag 1 : i32
+ test.op_with_bit_enum_space_directive other user tag 1 : i32
+ return
+}
+
+// -----
+
+func.func @space_separated_case_missing() {
+ // expected-error at +2 {{expected one of [none, user, group, other] for a space-separated test bit enum}}
+ // expected-error at +1 {{failed to parse TestBitEnumSpaceAttr}}
+ "test.op_with_bit_enum_space"() {value = #test.bit_enum_space<sticky>} : () -> ()
+}
+
+// -----
+
func.func @disallowed_case_sticky_fail() {
// expected-error at +2 {{expected one of [none, read, write, execute] for a test bit enum, got: sticky}}
// expected-error at +1 {{failed to parse TestBitEnumAttr}}
diff --git a/mlir/test/IR/enum-attr-roundtrip.mlir b/mlir/test/IR/enum-attr-roundtrip.mlir
index 9b4b8de898942f..c3a0e1446f851f 100644
--- a/mlir/test/IR/enum-attr-roundtrip.mlir
+++ b/mlir/test/IR/enum-attr-roundtrip.mlir
@@ -123,6 +123,30 @@ func.func @test_two_default_bit_enum_props() -> () {
return
}
+// CHECK-LABEL: @test_space_bit_enum_prop
+func.func @test_space_bit_enum_prop() -> () {
+ // CHECK: test.op_with_default_space_bit_enum_prop{{$}}
+ test.op_with_default_space_bit_enum_prop
+
+ // CHECK: test.op_with_default_space_bit_enum_prop user group{{$}}
+ test.op_with_default_space_bit_enum_prop group user
+
+ // Parsing stops at the first keyword that is not a case.
+ // CHECK: test.op_with_default_space_bit_enum_prop other tag 1 : i32{{$}}
+ test.op_with_default_space_bit_enum_prop other tag 1 : i32
+
+ // CHECK: test.op_with_default_space_bit_enum_prop tag 1 : i32{{$}}
+ test.op_with_default_space_bit_enum_prop tag 1 : i32
+
+ // An explicit `none` is the default and so is elided.
+ // CHECK: test.op_with_default_space_bit_enum_prop{{$}}
+ test.op_with_default_space_bit_enum_prop none
+
+ // CHECK: test.op_with_default_space_bit_enum_prop user other{{$}}
+ "test.op_with_default_space_bit_enum_prop"() <{value = 5 : i32}> : () -> ()
+ return
+}
+
// CHECK-LABEL @test_bit_enum_prop()
func.func @test_bit_enum_prop() -> () {
// CHECK: test.op_with_bit_enum_prop read : ()
diff --git a/mlir/test/lib/Dialect/Test/TestEnumDefs.td b/mlir/test/lib/Dialect/Test/TestEnumDefs.td
index daf898ca7ee061..c0a600120a3871 100644
--- a/mlir/test/lib/Dialect/Test/TestEnumDefs.td
+++ b/mlir/test/lib/Dialect/Test/TestEnumDefs.td
@@ -163,6 +163,18 @@ def TestBitEnumVerticalBar
let separator = " | ";
}
+// Define an enum whose cases are separated by whitespace only.
+def TestBitEnumSpace
+ : I32BitEnum<"TestBitEnumSpace", "a space-separated test bit enum", [
+ TestBitEnumNone,
+ I32BitEnumCaseBit<"User", 0, "user">,
+ I32BitEnumCaseBit<"Group", 1, "group">,
+ I32BitEnumCaseBit<"Other", 2, "other">,
+ ]> {
+ let cppNamespace = "test";
+ let separator = " ";
+}
+
def TestStructBitEnum
: I32BitEnum<"TestStructBitEnum", "a bit enum nested in a struct", [
TestBitEnumNone,
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index fc096a9e61d710..07bb7d706f98ac 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -612,6 +612,21 @@ def OpWithBitEnumVerticalBarDirective
let assemblyFormat = "enum($value) attr-dict";
}
+def TestBitEnumSpaceAttr
+ : EnumAttr<Test_Dialect, TestBitEnumSpace, "bit_enum_space">;
+
+def OpWithBitEnumSpace : TEST_Op<"op_with_bit_enum_space"> {
+ let arguments = (ins TestBitEnumSpaceAttr:$value,
+ OptionalAttr<AnyAttr>:$tag);
+ let assemblyFormat = "$value (`tag` $tag^)? attr-dict";
+}
+
+def OpWithBitEnumSpaceDirective : TEST_Op<"op_with_bit_enum_space_directive"> {
+ let arguments = (ins TestBitEnumSpaceAttr:$value,
+ OptionalAttr<AnyAttr>:$tag);
+ let assemblyFormat = "enum($value) (`tag` $tag^)? attr-dict";
+}
+
// Define a pattern that matches and creates a bit enum attribute.
def : Pat<(OpWithBitEnum ConstantEnumCase<TestBitEnumAttr, "write|execute">,
ConstantAttr<I32Attr, "0">),
diff --git a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
index f2d6ba3c5855e5..115f8734b03e42 100644
--- a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
+++ b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
@@ -484,6 +484,16 @@ def OpWithTwoDefaultBitEnumProps : TEST_Op<"op_with_two_default_bit_enum_props">
let assemblyFormat = "($value1^)? ($value2^)? attr-dict";
}
+def TestBitEnumSpacePropDefault : EnumProp<TestBitEnumSpace> {
+ let defaultValue = TestBitEnumSpace.cppType # "::None";
+}
+def OpWithDefaultSpaceBitEnumProp
+ : TEST_Op<"op_with_default_space_bit_enum_prop"> {
+ let arguments = (ins TestBitEnumSpacePropDefault:$value,
+ OptionalAttr<AnyAttr>:$tag);
+ let assemblyFormat = "($value^)? (`tag` $tag^)? attr-dict";
+}
+
def FormatOptionalWithElse : TEST_Op<"format_optional_else"> {
let arguments = (ins UnitAttr:$isFirstBranchPresent);
let assemblyFormat = "(`then` $isFirstBranchPresent^):(`else`)? attr-dict";
diff --git a/mlir/test/mlir-tblgen/enums-gen.td b/mlir/test/mlir-tblgen/enums-gen.td
index dc0caab3652d9f..97500e165b42e1 100644
--- a/mlir/test/mlir-tblgen/enums-gen.td
+++ b/mlir/test/mlir-tblgen/enums-gen.td
@@ -175,6 +175,26 @@ def MyCommaSeparatedBitEnum
let separator = ", ";
}
+def MySpaceSeparatedBitEnum
+ : I32BitEnum<"MySpaceSeparatedBitEnum", "Space-separated bit enum",
+ [None, Bit0, Bit1]> {
+ let separator = " ";
+}
+
+// DEF-LABEL: ::std::optional<MySpaceSeparatedBitEnum> symbolizeMySpaceSeparatedBitEnum(::llvm::StringRef str) {
+// DEF: str.split(symbols, ' ', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+
+// DEF-LABEL: FailureOr<::MySpaceSeparatedBitEnum> FieldParser<::MySpaceSeparatedBitEnum, ::MySpaceSeparatedBitEnum>::parse(AsmParser &parser) {
+// DEF: FailureOr<std::optional<::MySpaceSeparatedBitEnum>> flags =
+// DEF-NEXT: FieldParser<std::optional<::MySpaceSeparatedBitEnum>>::parse(parser);
+// DEF: if (!*flags)
+// DEF-NEXT: return parser.emitError(loc, "expected one of [none, tagged, Bit1] for Space-separated bit enum");
+// DEF: FieldParser<std::optional<::MySpaceSeparatedBitEnum>, std::optional<::MySpaceSeparatedBitEnum>>::parse(AsmParser &parser) {
+// DEF: std::optional<::MySpaceSeparatedBitEnum> flags;
+// DEF: while (::mlir::succeeded(parser.parseOptionalKeyword(&enumKeyword, {"none", "tagged", "Bit1"})))
+// DEF-NEXT: flags = flags.value_or(::MySpaceSeparatedBitEnum{}) | *::symbolizeEnum<::MySpaceSeparatedBitEnum>(enumKeyword);
+// DEF-NEXT: return flags;
+
// DECL: inline ::llvm::raw_ostream &operator<<(::llvm::raw_ostream &p, ::MyNonQuotedPrintBitEnum value) {
// DECL: auto valueStr = stringifyEnum(value);
// DECL-NEXT: return p << valueStr;
diff --git a/mlir/test/mlir-tblgen/op-format-invalid.td b/mlir/test/mlir-tblgen/op-format-invalid.td
index 1b78d8177091fb..a8901fe96ecd0e 100644
--- a/mlir/test/mlir-tblgen/op-format-invalid.td
+++ b/mlir/test/mlir-tblgen/op-format-invalid.td
@@ -50,6 +50,15 @@ def DirectiveCommaBitEnum
def DirectiveCommaBitEnumAttr
: EnumAttr<TestDialect, DirectiveCommaBitEnum, "comma_bit_enum">;
+def DirectiveSpaceBitEnum
+ : I32BitEnum<"DirectiveSpaceBitEnum", "space bit enum",
+ [DirectiveBitEnumNone, DirectiveBitEnumCase]> {
+ let cppNamespace = "::test";
+ let separator = " ";
+}
+def DirectiveSpaceBitEnumAttr
+ : EnumAttr<TestDialect, DirectiveSpaceBitEnum, "space_bit_enum">;
+
def DirectiveNoZeroBitEnum
: I32BitEnum<"DirectiveNoZeroBitEnum", "bit enum without zero",
[DirectiveBitEnumCase]> {
@@ -98,6 +107,11 @@ def DirectiveAEnumInvalidH : TestFormat_Op<[{
enum($attr) `,` attr-dict
}]>, Arguments<(ins DirectiveCommaBitEnumAttr:$attr)>;
+// CHECK: error: space-separated bit enum cannot be followed by its 'case' case literal
+def DirectiveAEnumInvalidI : TestFormat_Op<[{
+ enum($attr) `case` attr-dict
+}]>, Arguments<(ins DirectiveSpaceBitEnumAttr:$attr)>;
+
//===----------------------------------------------------------------------===//
// attr-dict
//===----------------------------------------------------------------------===//
diff --git a/mlir/tools/mlir-tblgen/EnumsGen.cpp b/mlir/tools/mlir-tblgen/EnumsGen.cpp
index c83ed3cf42fcd7..8aef2efd6f2626 100644
--- a/mlir/tools/mlir-tblgen/EnumsGen.cpp
+++ b/mlir/tools/mlir-tblgen/EnumsGen.cpp
@@ -223,10 +223,41 @@ FieldParser<std::optional<{0}>, std::optional<{0}>>::parse(AsmParser &parser) {{
return std::optional<{0}>{{flags};
}
} // namespace mlir
+)";
+
+ // With a space-only separator, the flags are a run of case keywords that
+ // ends at the first token that is not one.
+ const char *const fieldParserDefsSpaceBitEnum = R"(
+namespace mlir {
+FailureOr<{0}> FieldParser<{0}, {0}>::parse(AsmParser &parser) {{
+ auto loc = parser.getCurrentLocation();
+ FailureOr<std::optional<{0}>> flags =
+ FieldParser<std::optional<{0}>>::parse(parser);
+ if (failed(flags))
+ return failure();
+ if (!*flags)
+ return parser.emitError(loc, "expected one of {3} for {2}");
+ return **flags;
+}
+
+FailureOr<std::optional<{0}>>
+FieldParser<std::optional<{0}>, std::optional<{0}>>::parse(AsmParser &parser) {{
+ std::optional<{0}> flags;
+ ::llvm::StringRef enumKeyword;
+ while (::mlir::succeeded(parser.parseOptionalKeyword(&enumKeyword, {4})))
+ flags = flags.value_or({0}{{}) | *{1}::symbolizeEnum<{0}>(enumKeyword);
+ return flags;
+}
+} // namespace mlir
)";
if (enumInfo.isNewStyleBitEnum()) {
StringRef separator = enumInfo.getDef().getValueAsString("separator");
+ if (separator.trim().empty()) {
+ os << formatv(fieldParserDefsSpaceBitEnum, qualName, cppNamespace,
+ enumInfo.getSummary(), casesList, casesInitList);
+ return;
+ }
StringRef parseSeparatorFn =
llvm::StringSwitch<StringRef>(separator.trim())
.Case("|", "parseOptionalVerticalBar")
@@ -579,8 +610,13 @@ static void emitStrToSymFnForBitEnum(const Record &enumDef, raw_ostream &os) {
// Split the string to get symbols for all the bits.
os << " ::llvm::SmallVector<::llvm::StringRef, 2> symbols;\n";
- // Remove whitespace from the separator string when parsing.
- os << formatv(" str.split(symbols, \"{0}\");\n\n", separatorTrimmed);
+ // Remove whitespace from the separator string when parsing. A space-only
+ // separator splits on spaces and drops the empty pieces.
+ if (separatorTrimmed.empty())
+ os << " str.split(symbols, ' ', /*MaxSplit=*/-1, "
+ "/*KeepEmpty=*/false);\n\n";
+ else
+ os << formatv(" str.split(symbols, \"{0}\");\n\n", separatorTrimmed);
os << formatv(" {0} val = 0;\n", underlyingType);
os << " for (auto symbol : symbols) {\n";
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index 1fd993cc197a9a..e7798cb368e3cb 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -614,7 +614,8 @@ const char *const enumAttrParserCode = R"(
/// {5}: The error message on failure when the enum isn't present.
/// {6}: The attribute assignment expression.
/// {7}: The C++ enum type.
-/// {8}: The optional parser for the enum separator.
+/// {8}: The code that parses the next case keyword into `attrStr`, breaking
+/// out of the loop when there is none.
const char *const unquotedBitEnumAttrParserCode = R"(
{
::llvm::StringRef attrStr;
@@ -629,10 +630,7 @@ const char *const unquotedBitEnumAttrParserCode = R"(
return parser.emitError(loc, "invalid ")
<< "{0} attribute specification: \"" << attrStr << '"';;
flags = flags | *flag;
- if (failed(parser.{8}()))
- break;
- if (parser.parseKeyword(&attrStr))
- return ::mlir::failure();
+ {8}
}
auto attrOptional = ::std::optional<{7}>(flags);
{0}Attr = {3};
@@ -1311,18 +1309,32 @@ static void genEnumAttrParser(const NamedAttribute *var, MethodBody &body,
if (formatAsEnumDirective && enumInfo.isBitEnum() &&
(formatBitEnumAsUnquoted || !enumInfo.printBitEnumQuoted())) {
- StringRef separator = enumInfo.getDef().getValueAsString("separator");
- StringRef parseSeparatorFn = llvm::StringSwitch<StringRef>(separator.trim())
- .Case("|", "parseOptionalVerticalBar")
- .Case(",", "parseOptionalComma")
- .Default("invalid bit enum separator");
+ // With a space-only separator, the cases are a run of keywords;
+ // otherwise each further case follows a separator token.
+ StringRef separator =
+ enumInfo.getDef().getValueAsString("separator").trim();
+ std::string parseNextCase;
+ if (separator.empty()) {
+ parseNextCase =
+ formatv("if (parser.parseOptionalKeyword(&attrStr, {0})) break;",
+ validCaseKeywordsStr);
+ } else {
+ StringRef parseSeparatorFn = llvm::StringSwitch<StringRef>(separator)
+ .Case("|", "parseOptionalVerticalBar")
+ .Case(",", "parseOptionalComma")
+ .Default("invalid bit enum separator");
+ parseNextCase = formatv("if (failed(parser.{0}())) break;\n"
+ " if (parser.parseKeyword(&attrStr))\n"
+ " return ::mlir::failure();",
+ parseSeparatorFn);
+ }
std::string enumType =
(enumInfo.getCppNamespace() + "::" + enumInfo.getEnumClassName()).str();
body << formatv(unquotedBitEnumAttrParserCode, var->name,
enumInfo.getCppNamespace(),
enumInfo.getStringToSymbolFnName(), attrBuilderStr,
validCaseKeywordsStr, errorMessage, attrAssignment,
- enumType, parseSeparatorFn);
+ enumType, parseNextCase);
return;
}
@@ -4008,6 +4020,16 @@ OpFormatParser::verifyEnumAdjacentLiterals(SMLoc loc,
return emitError(loc, "unquoted bit enum cannot be followed by its '" +
separator + "' separator literal");
}
+ // With a space-only separator, the parser would take a following case
+ // literal as another flag.
+ if (separator.empty() &&
+ llvm::any_of(enumInfo.getAllCases(), [&](const EnumCase &enumCase) {
+ return enumCase.getStr() == literal->getSpelling();
+ })) {
+ return emitError(loc, "space-separated bit enum cannot be followed by "
+ "its '" +
+ literal->getSpelling() + "' case literal");
+ }
}
return success();
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/237149
More information about the Mlir-commits
mailing list