[Mlir-commits] [mlir] [MLIR][ODS] Add separator support to oilist (PR #217891)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Fri Aug 21 10:51:16 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-core
Author: Mehdi Amini (joker-eph)
<details>
<summary>Changes</summary>
Allow an optional literal separator in oilist formats. Parse and print separators only between present clauses, and diagnose missing, trailing, and ambiguous separators.
Assisted-by: Codex
---
Full diff: https://github.com/llvm/llvm-project/pull/217891.diff
6 Files Affected:
- (modified) mlir/docs/DefiningDialects/Operations.md (+6-1)
- (modified) mlir/test/IR/traits.mlir (+36)
- (modified) mlir/test/lib/Dialect/Test/TestOpsSyntax.td (+10)
- (modified) mlir/test/mlir-tblgen/op-format-invalid.td (+20)
- (modified) mlir/test/mlir-tblgen/op-format-spec.td (+4)
- (modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+94-7)
``````````diff
diff --git a/mlir/docs/DefiningDialects/Operations.md b/mlir/docs/DefiningDialects/Operations.md
index 419041a52b0da..75ddd92492642 100644
--- a/mlir/docs/DefiningDialects/Operations.md
+++ b/mlir/docs/DefiningDialects/Operations.md
@@ -786,10 +786,15 @@ The available directives are as follows:
- The constraints on `inputs` and `outputs` are the same as the `input` of
the `type` directive.
-* ``oilist ( `keyword` elements | `otherKeyword` elements ...)``
+* ``oilist ( `keyword` elements | `otherKeyword` elements ...)`` or
+ ``oilist < `separator` > ( `keyword` elements | `otherKeyword` elements ...)``
- Represents an optional order-independent list of clauses. Each clause
has a keyword and corresponding assembly format.
+ - The separator specification is optional. When present, the separator
+ is parsed and printed between clauses. For example,
+ ``oilist<`,`>(...)`` formats a comma-separated list without a trailing
+ comma.
- Each clause can appear 0 or 1 time (in any order).
- Only literals, types and variables can be used within an oilist element.
- All the variables must be optional or variadic.
diff --git a/mlir/test/IR/traits.mlir b/mlir/test/IR/traits.mlir
index b20edde07a6c7..5e5c2afe88cbd 100644
--- a/mlir/test/IR/traits.mlir
+++ b/mlir/test/IR/traits.mlir
@@ -654,6 +654,42 @@ func.func @succeededOilistCustom(%arg0: i32, %arg1: i32, %arg2: i32) {
// -----
+// CHECK-LABEL: @succeededOilistWithSeparator
+func.func @succeededOilistWithSeparator() {
+ // CHECK: test.oilist_with_separator
+ test.oilist_with_separator
+ // CHECK: test.oilist_with_separator keyword
+ test.oilist_with_separator keyword
+ // CHECK: test.oilist_with_separator keyword, otherKeyword
+ test.oilist_with_separator otherKeyword, keyword
+ // CHECK: test.oilist_with_separator keyword, otherKeyword, thirdKeyword
+ test.oilist_with_separator thirdKeyword, keyword, otherKeyword
+ return
+}
+
+// -----
+
+func.func @failedOilistWithDuplicateSeparatedClause() {
+ // expected-error at +1 {{`keyword` clause can appear at most once in the expansion of the oilist directive}}
+ test.oilist_with_separator keyword, keyword
+}
+
+// -----
+
+func.func @failedOilistWithMissingSeparator() {
+ // expected-error at +1 {{expected ',' between oilist clauses}}
+ test.oilist_with_separator keyword otherKeyword
+}
+
+// -----
+
+func.func @failedOilistWithTrailingSeparator() {
+ // expected-error at +1 {{expected oilist clause after separator}}
+ test.oilist_with_separator keyword,
+}
+
+// -----
+
func.func @failedHasDominanceScopeOutsideDominanceFreeScope() -> () {
"test.ssacfg_region"() ({
test.graph_region {
diff --git a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
index 1d9ff9bdef3a2..3ab3c8b9db6e7 100644
--- a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
+++ b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
@@ -137,6 +137,16 @@ def OIListCustom : TEST_Op<"oilist_custom", [AttrSizedOperandSegments]> {
}];
}
+def OIListWithSeparator : TEST_Op<"oilist_with_separator"> {
+ let arguments = (ins UnitAttr:$keyword, UnitAttr:$otherKeyword,
+ UnitAttr:$diffNameUnitAttrKeyword);
+ let assemblyFormat = [{
+ oilist<`,`>( `keyword` $keyword
+ | `otherKeyword` $otherKeyword
+ | `thirdKeyword` $diffNameUnitAttrKeyword) attr-dict
+ }];
+}
+
def OIListAllowedLiteral : TEST_Op<"oilist_allowed_literal"> {
let assemblyFormat = [{
oilist( `foo` | `bar` ) `buzz` attr-dict
diff --git a/mlir/test/mlir-tblgen/op-format-invalid.td b/mlir/test/mlir-tblgen/op-format-invalid.td
index 1944bc4feb634..38c8d57a342b6 100644
--- a/mlir/test/mlir-tblgen/op-format-invalid.td
+++ b/mlir/test/mlir-tblgen/op-format-invalid.td
@@ -372,10 +372,30 @@ def OIListErrorNoLiteral : TestFormat_Op<[{
def OIListLiteralAmbiguity : TestFormat_Op<[{
oilist( `foo` | `bar` ) `foo` attr-dict
}]>;
+// CHECK: error: format ambiguity because foo is used as both an oilist separator and clause keyword.
+def OIListSeparatorClauseAmbiguity : TestFormat_Op<[{
+ oilist<`foo`>(`foo`) attr-dict
+}]>;
+// CHECK: error: format ambiguity because , is used both in oilist element and the adjacent literal.
+def OIListSeparatorLiteralAmbiguity : TestFormat_Op<[{
+ oilist<`,`>(`foo`) `,` attr-dict
+}]>;
+// CHECK: error: expected literal, but got '>'
+def OIListSeparatorMissing : TestFormat_Op<[{
+ oilist<>(`foo`) attr-dict
+}]>;
// CHECK: error: expected '(' before oilist argument list
def OIListStartingToken : TestFormat_Op<[{
oilist `wrong` attr-dict
}]>;
+// CHECK: error: expected '>' after oilist separator
+def OIListUnterminatedSeparator : TestFormat_Op<[{
+ oilist<`,` (`foo`) attr-dict
+}]>;
+// CHECK: error: oilist separator must be a non-whitespace literal
+def OIListWhitespaceSeparator : TestFormat_Op<[{
+ oilist<` `>(`foo`) attr-dict
+}]>;
//===----------------------------------------------------------------------===//
// Optional Groups
diff --git a/mlir/test/mlir-tblgen/op-format-spec.td b/mlir/test/mlir-tblgen/op-format-spec.td
index 0ad6d0959809d..fac4386f92f80 100644
--- a/mlir/test/mlir-tblgen/op-format-spec.td
+++ b/mlir/test/mlir-tblgen/op-format-spec.td
@@ -152,6 +152,10 @@ def OIListCustom : TestFormat_Op<[{
| `nowait`
| `reduction` custom<ReductionClause>($arg1, type($arg1))) attr-dict
}], [AttrSizedOperandSegments]>, Arguments<(ins Optional<AnyType>:$arg0, Optional<AnyType>:$arg1)>;
+def OIListWithSeparator : TestFormat_Op<[{
+ oilist<`,`>( `keyword` $arg0 `:` type($arg0)
+ | `otherkeyword` $arg1 `:` type($arg1)) attr-dict
+}], [AttrSizedOperandSegments]>, Arguments<(ins Optional<AnyType>:$arg0, Optional<AnyType>:$arg1)>;
//===----------------------------------------------------------------------===//
// Optional Groups
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index 815c51c91dddd..6695179a05cf4 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -224,16 +224,20 @@ class TypeDirective : public DirectiveElementBase<DirectiveElement::Type> {
};
/// This class represents a group of order-independent optional clauses. Each
-/// clause starts with a literal element and has a coressponding parsing
-/// element. A parsing element is a continous sequence of format elements.
-/// Each clause can appear 0 or 1 time.
+/// clause starts with a literal element and has a corresponding parsing
+/// element. A parsing element is a continuous sequence of format elements.
+/// Each clause can appear 0 or 1 time. An optional literal separates clauses.
class OIListElement : public DirectiveElementBase<DirectiveElement::OIList> {
public:
- OIListElement(std::vector<FormatElement *> &&literalElements,
+ OIListElement(LiteralElement *separator,
+ std::vector<FormatElement *> &&literalElements,
std::vector<std::vector<FormatElement *>> &&parsingElements)
- : literalElements(std::move(literalElements)),
+ : separator(separator), literalElements(std::move(literalElements)),
parsingElements(std::move(parsingElements)) {}
+ /// Returns the optional separator between clauses.
+ LiteralElement *getSeparator() const { return separator; }
+
/// Returns a range to iterate over the LiteralElements.
auto getLiteralElements() const {
return llvm::map_range(literalElements, [](FormatElement *el) {
@@ -265,6 +269,9 @@ class OIListElement : public DirectiveElementBase<DirectiveElement::OIList> {
}
private:
+ /// An optional literal printed and parsed between clauses.
+ LiteralElement *separator;
+
/// A vector of `LiteralElement` objects. Each element stores the keyword
/// for one case of oilist element. For example, an oilist element along with
/// the `literalElements` vector:
@@ -1851,11 +1858,41 @@ void OperationFormat::genElementParser(FormatElement *element, MethodBody &body,
/// OIList Directive
} else if (OIListElement *oilist = dyn_cast<OIListElement>(element)) {
+ if (oilist->getSeparator()) {
+ body << " {\n";
+ body.indent();
+ }
+
for (LiteralElement *le : oilist->getLiteralElements())
body << " bool " << le->getSpelling() << "Clause = false;\n";
+ if (oilist->getSeparator())
+ body << " bool oilistClauseParsed = false;\n";
// Generate the parsing loop
body << " while(true) {\n";
+ if (LiteralElement *separator = oilist->getSeparator()) {
+ body << " auto oilistSeparatorLoc = parser.getCurrentLocation();\n";
+ body << " if (oilistClauseParsed) {\n";
+ body << " if (failed(parser.parseOptional";
+ genLiteralParser(separator->getSpelling(), body);
+ body << ")) {\n";
+ body << " if (";
+ llvm::interleave(
+ oilist->getLiteralElements(),
+ [&](LiteralElement *literal) {
+ body << "succeeded(parser.parseOptional";
+ genLiteralParser(literal->getSpelling(), body);
+ body << ")";
+ },
+ [&] { body << " || "; });
+ body << ")\n";
+ body << " return parser.emitError(oilistSeparatorLoc,\n"
+ " \"expected '"
+ << separator->getSpelling() << "' between oilist clauses\");\n";
+ body << " break;\n";
+ body << " }\n";
+ body << " }\n";
+ }
for (auto clause : oilist->getClauses()) {
LiteralElement *lelement = std::get<0>(clause);
ArrayRef<FormatElement *> pelement = std::get<1>(clause);
@@ -1864,6 +1901,8 @@ void OperationFormat::genElementParser(FormatElement *element, MethodBody &body,
body << ")) {\n";
StringRef lelementName = lelement->getSpelling();
body << formatv(oilistParserCode, lelementName);
+ if (oilist->getSeparator())
+ body << " oilistClauseParsed = true;\n";
if (AttributeLikeVariable *unitVarElem =
oilist->getUnitVariableParsingElement(pelement)) {
if (isa<PropertyVariable>(unitVarElem)) {
@@ -1883,9 +1922,16 @@ void OperationFormat::genElementParser(FormatElement *element, MethodBody &body,
body << " } else ";
}
body << " {\n";
+ if (oilist->getSeparator()) {
+ body << " if (oilistClauseParsed)\n";
+ body << " return parser.emitError(oilistSeparatorLoc,\n"
+ " \"expected oilist clause after separator\");\n";
+ }
body << " break;\n";
body << " }\n";
body << "}\n";
+ if (oilist->getSeparator())
+ body.unindent() << " }\n";
/// Literals.
} else if (LiteralElement *literal = dyn_cast<LiteralElement>(element)) {
@@ -2782,6 +2828,11 @@ void OperationFormat::genElementPrinter(FormatElement *element,
// Emit the OIList
if (auto *oilist = dyn_cast<OIListElement>(element)) {
+ if (oilist->getSeparator()) {
+ body << " {\n";
+ body.indent();
+ body << " bool oilistClausePrinted = false;\n";
+ }
for (auto clause : oilist->getClauses()) {
LiteralElement *lelement = std::get<0>(clause);
ArrayRef<FormatElement *> pelement = std::get<1>(clause);
@@ -2824,6 +2875,13 @@ void OperationFormat::genElementPrinter(FormatElement *element,
}
body << ") {\n";
+ if (LiteralElement *separator = oilist->getSeparator()) {
+ body << " if (oilistClausePrinted) {\n";
+ genLiteralPrinter(separator->getSpelling(), body, shouldEmitSpace,
+ lastWasPunctuation);
+ body << " }\n";
+ body << " oilistClausePrinted = true;\n";
+ }
genLiteralPrinter(lelement->getSpelling(), body, shouldEmitSpace,
lastWasPunctuation);
if (oilist->getUnitVariableParsingElement(pelement) == nullptr) {
@@ -2833,6 +2891,8 @@ void OperationFormat::genElementPrinter(FormatElement *element,
}
body << " }\n";
}
+ if (oilist->getSeparator())
+ body.unindent() << " }\n";
return;
}
@@ -3561,10 +3621,20 @@ LogicalResult OpFormatParser::verifySuccessors(SMLoc loc) {
LogicalResult
OpFormatParser::verifyOIListElements(SMLoc loc,
ArrayRef<FormatElement *> elements) {
- // Check that all of the successors are within the format.
+ // Check for ambiguous literals in and around oilist elements.
SmallVector<StringRef> prohibitedLiterals;
for (FormatElement *it : elements) {
if (auto *oilist = dyn_cast<OIListElement>(it)) {
+ if (LiteralElement *separator = oilist->getSeparator()) {
+ for (LiteralElement *literal : oilist->getLiteralElements()) {
+ if (literal->getSpelling() == separator->getSpelling()) {
+ return emitError(
+ loc, "format ambiguity because " + separator->getSpelling() +
+ " is used as both an oilist separator and clause "
+ "keyword.");
+ }
+ }
+ }
if (!prohibitedLiterals.empty()) {
// We just saw an oilist element in last iteration. Literals should not
// match.
@@ -3579,6 +3649,8 @@ OpFormatParser::verifyOIListElements(SMLoc loc,
}
for (LiteralElement *literal : oilist->getLiteralElements())
prohibitedLiterals.push_back(literal->getSpelling());
+ if (LiteralElement *separator = oilist->getSeparator())
+ prohibitedLiterals.push_back(separator->getSpelling());
} else if (auto *literal = dyn_cast<LiteralElement>(it)) {
if (find(prohibitedLiterals, literal->getSpelling()) !=
prohibitedLiterals.end()) {
@@ -3935,6 +4007,21 @@ OpFormatParser::parseSuccessorsDirective(SMLoc loc, Context context) {
FailureOr<FormatElement *>
OpFormatParser::parseOIListDirective(SMLoc loc, Context context) {
+ LiteralElement *separator = nullptr;
+ if (peekToken().is(FormatToken::less)) {
+ consumeToken();
+ SMLoc separatorLoc = peekToken().getLoc();
+ FailureOr<FormatElement *> separatorElement = parseLiteral(context);
+ if (failed(separatorElement))
+ return failure();
+ separator = dyn_cast<LiteralElement>(*separatorElement);
+ if (!separator)
+ return emitError(separatorLoc,
+ "oilist separator must be a non-whitespace literal");
+ if (failed(parseToken(FormatToken::greater,
+ "expected '>' after oilist separator")))
+ return failure();
+ }
if (failed(parseToken(FormatToken::l_paren,
"expected '(' before oilist argument list")))
return failure();
@@ -3965,7 +4052,7 @@ OpFormatParser::parseOIListDirective(SMLoc loc, Context context) {
}
} while (true);
- return create<OIListElement>(std::move(literalElements),
+ return create<OIListElement>(separator, std::move(literalElements),
std::move(parsingElements));
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/217891
More information about the Mlir-commits
mailing list