[Mlir-commits] [mlir] [mlir]Add resultSegmentSizes/operandSegmentSizes to prop-dict (PR #212486)
Natanael Cintean
llvmlistbot at llvm.org
Tue Jul 28 06:23:40 PDT 2026
https://github.com/natanael-cintean created https://github.com/llvm/llvm-project/pull/212486
PR merged on main llvm.
Fixes https://github.com/llvm/llvm-project/issues/211220
setPropertiesFromParsedAttr (generated by OpFormatGen.cpp for ops using a
custom assemblyFormat) rejects the trait-injected operandSegmentSizes /
resultSegmentSizes properties when they appear in a prop-dict, even though
the printer emits them there for ops whose format uses a bulk operand/result
type directive (type(operands), type(results),
functional-type(operands, results)).
This breaks round-tripping: any op combining AttrSizedOperandSegments /
AttrSizedResultSegments with such a bulk directive fails to re-parse its own
printed output, because printer-side elision of these keys
(introduced in https://github.com/llvm/llvm-project/pull/115930) is intentionally skipped in that case — the sizes
can't be reconstructed from individually-typed operand/result groups, so they
must survive in the text, but the custom parser was never taught to read them
back.
setPropertiesFromAttr (used by the generic-form parser, bytecode, and C++
construction) already special-cases these two keys, so this change makes
setPropertiesFromParsedAttr mirror that behavior: keys are accepted from the
dictionary attr when the corresponding trait is present, and required exactly
when the format can't reconstruct the sizes itself (i.e. when
fmt.allOperands / fmt.allResultTypes is true). Otherwise the key remains
optional, since genParserVariadicSegmentResolution overwrites it with the
sizes inferred from the parsed operand/result groups.
The contents of the PR as well as description has been made using github-copilot. However I've reviewed the contents and tested locally the change.
For the PR description I've kept the AI suggestion as to me it seemed that the informations are needed to quickly understand the underlying issue.
>From 41835df006b9b5146dda44d7e2824b5e0a2baaa9 Mon Sep 17 00:00:00 2001
From: Natanael Cintean <natanael.cintean at intel.com>
Date: Tue, 21 Jul 2026 14:56:22 +0000
Subject: [PATCH 1/4] Add resultSegmentSizes/operandSegmentSizes to prop-dict.
Add test to check for these in prop-dict.
---
mlir/test/IR/properties.mlir | 11 +++++++++
mlir/test/lib/Dialect/Test/TestOps.td | 16 +++++++++++++
mlir/tools/mlir-tblgen/OpFormatGen.cpp | 32 ++++++++++++++++++++++++++
3 files changed, 59 insertions(+)
diff --git a/mlir/test/IR/properties.mlir b/mlir/test/IR/properties.mlir
index 4dbb8ebfa5db3..4d83038f31cdd 100644
--- a/mlir/test/IR/properties.mlir
+++ b/mlir/test/IR/properties.mlir
@@ -46,6 +46,17 @@ test.using_property_ref_in_custom 1 + 4 = 5
%ci64 = arith.constant 0 : i64
test.variadic_segment_prop %ci64, %ci64 : %ci64 : i64, i64 : i64 end
+// Tests that the variadic segment size properties survive a round-trip
+// through the *custom* (non-generic) parser/printer when the assembly format
+// uses a bulk `functional-type(operands, results)` directive, which prevents
+// the printer from eliding `operandSegmentSizes` / `resultSegmentSizes` from
+// `<{...}>`. Without the parser-side fix, re-parsing the CHECK line below
+// (which is exactly what the printer emits) fails with "duplicate or unknown
+// key 'operandSegmentSizes' in dictionary attribute".
+// CHECK: test.variadic_segment_prop_bulk_type(%[[CI64]], %[[CI64]], %[[CI64]]) : (i64, i64, i64) -> (i64, i64, i64) <{operandSegmentSizes = array<i32: 2, 1>, resultSegmentSizes = array<i32: 2, 1>}>
+// GENERIC: "test.variadic_segment_prop_bulk_type"(%[[CI64]], %[[CI64]], %[[CI64]]) <{operandSegmentSizes = array<i32: 2, 1>, resultSegmentSizes = array<i32: 2, 1>}> : (i64, i64, i64) -> (i64, i64, i64)
+test.variadic_segment_prop_bulk_type(%ci64, %ci64, %ci64) : (i64, i64, i64) -> (i64, i64, i64) <{operandSegmentSizes = array<i32: 2, 1>, resultSegmentSizes = array<i32: 2, 1>}>
+
// CHECK: test.with_default_valued_properties na{{$}}
// GENERIC: "test.with_default_valued_properties"()
// GENERIC-SAME: <{a = 0 : i32, b = "", c = -1 : i32, unit = false}> : () -> ()
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index c12e2abb3ab75..db55b90c294e1 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -3686,6 +3686,22 @@ def TestOpWithVariadicSegmentProperties : TEST_Op<"variadic_segment_prop",
}];
}
+// Same as `TestOpWithVariadicSegmentProperties`, but the assembly format uses
+// the bulk `type(operands)` / `type(results)` directives instead of spelling
+// out the type of each individual variadic group. This prevents the printer
+// from eliding `operandSegmentSizes` / `resultSegmentSizes` from the
+// `<{...}>` properties dictionary (since there is no other way to recover the
+// segment sizes on re-parse), so this op exercises the parser's ability to
+// read those trait-injected properties back out of `prop-dict`.
+def TestOpWithBulkTypesAndSegmentProperties : TEST_Op<"variadic_segment_prop_bulk_type",
+ [AttrSizedOperandSegments, AttrSizedResultSegments]> {
+ let arguments = (ins Variadic<I64>:$a1, Variadic<I64>:$a2);
+ let results = (outs Variadic<I64>:$b1, Variadic<I64>:$b2);
+ let assemblyFormat = [{
+ `(` operands `)` `:` functional-type(operands, results) prop-dict attr-dict
+ }];
+}
+
def TestOpUsingPropertyRefInCustom : TEST_Op<"using_property_ref_in_custom"> {
let assemblyFormat = "custom<IntProperty>($first) `+` custom<SumProperty>($second, ref($first)) attr-dict";
let arguments = (ins IntProp<"int64_t">:$first, IntProp<"int64_t">:$second);
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index cbcbc8e9bc102..f9d07f6b3ba0f 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1313,6 +1313,38 @@ ::mlir::MLIRContext *ctx = dict.getContext();
(void)ctx;
)decl";
+ // `operandSegmentSizes`/`resultSegmentSizes` are trait-injected properties
+ // not enumerated by `op.getProperties()`, so they need to be special-cased
+ // here, mirroring the handling in `setPropertiesFromAttr` in
+ // OpDefinitionsGen.cpp.
+ //
+ // {0}: segment sizes property name
+ // {1}: isRequired
+ const char *segmentSizesFromAttrFmt = R"decl(
+auto {0}AttrName = ::mlir::StringAttr::get(ctx, "{0}");
+usedKeys.insert({0}AttrName);
+auto attr = dict.get({0}AttrName);
+if (!attr && {1}) {{
+ emitError() << "expected key entry for {0} in DictionaryAttr to set "
+ "Properties.";
+ return ::mlir::failure();
+}
+if (attr && ::mlir::failed(::mlir::convertFromAttribute(prop.{0}, attr, [&]() {{
+ return emitError() << "for `{0}`: ";
+ })))
+ return ::mlir::failure();
+)decl";
+ if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) {
+ auto scope = body.scope("{\n", "}\n", /*indent=*/true);
+ body << formatv(segmentSizesFromAttrFmt, "operandSegmentSizes",
+ fmt.allOperands);
+ }
+ if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments")) {
+ auto scope = body.scope("{\n", "}\n", /*indent=*/true);
+ body << formatv(segmentSizesFromAttrFmt, "resultSegmentSizes",
+ fmt.allResultTypes);
+ }
+
// {0}: fromAttribute call
// {1}: property name
// {2}: isRequired
>From 446038c810667481033bb27862af18d61141d6be Mon Sep 17 00:00:00 2001
From: Natanael Cintean <natanael.cintean at intel.com>
Date: Tue, 28 Jul 2026 11:07:29 +0000
Subject: [PATCH 2/4] Add tests for invalid operandSegmentSizes as well as
required and infered cases.
---
mlir/test/IR/invalid-properties.mlir | 20 +++++++++++++++++
mlir/test/mlir-tblgen/op-format.td | 32 ++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/mlir/test/IR/invalid-properties.mlir b/mlir/test/IR/invalid-properties.mlir
index 2045db548c044..3678c98db43d1 100644
--- a/mlir/test/IR/invalid-properties.mlir
+++ b/mlir/test/IR/invalid-properties.mlir
@@ -43,6 +43,26 @@ func.func @wrong_dense_i32_array_prop_type() {
// -----
+// `operandSegmentSizes` is not required in `<{...}>` for this op, since its
+// format spells out each variadic group individually
+// If it is present anyway, it
+// must still be well-formed: it should not be silently ignored.
+func.func @malformed_optional_operand_segment_sizes(%arg0: i64) {
+ // expected-error at +1 {{for `operandSegmentSizes`: expected DenseI32ArrayAttr}}
+ test.variadic_segment_prop %arg0, %arg0 : %arg0 : i64, i64 : i64 <{operandSegmentSizes = "bad"}> end
+ return
+}
+
+// -----
+
+func.func @malformed_optional_result_segment_sizes(%arg0: i64) {
+ // expected-error at +1 {{for `resultSegmentSizes`: expected DenseI32ArrayAttr}}
+ test.variadic_segment_prop %arg0, %arg0 : %arg0 : i64, i64 : i64 <{resultSegmentSizes = "bad"}> end
+ return
+}
+
+// -----
+
func.func @valid_all_properties() {
"test.with_properties"() <{a = 32 : i64, array = array<i64: 1, 2, 3, 4>, array32 = array<i32: 5, 6>, b = "foo", c = "bar", flag = true}> : () -> ()
return
diff --git a/mlir/test/mlir-tblgen/op-format.td b/mlir/test/mlir-tblgen/op-format.td
index 1790737a3a349..7f388fd796f53 100644
--- a/mlir/test/mlir-tblgen/op-format.td
+++ b/mlir/test/mlir-tblgen/op-format.td
@@ -110,6 +110,38 @@ def OptionalGroupD : TestFormat_Op<[{
(custom<Custom>($a, $b)^)? attr-dict
}], [AttrSizedOperandSegments]>, Arguments<(ins Optional<I64>:$a, Optional<I64>:$b)>;
+//===----------------------------------------------------------------------===//
+// prop-dict + AttrSizedOperandSegments
+//===----------------------------------------------------------------------===//
+
+// When the format spells out each variadic operand group individually, the
+// segment sizes can be inferred from what was actually parsed, so the
+// `operandSegmentSizes` key is optional (but still validated if present).
+// CHECK-LABEL: PropDictSegmentSizesInferred::setPropertiesFromParsedAttr
+// CHECK: auto operandSegmentSizesAttrName = ::mlir::StringAttr::get(ctx, "operandSegmentSizes");
+// CHECK-NEXT: usedKeys.insert(operandSegmentSizesAttrName);
+// CHECK-NEXT: auto attr = dict.get(operandSegmentSizesAttrName);
+// CHECK-NEXT: if (!attr && false) {
+// CHECK: if (attr && ::mlir::failed(::mlir::convertFromAttribute(prop.operandSegmentSizes, attr,
+def PropDictSegmentSizesInferred : TestFormat_Op<[{
+ $a1 `:` $a2 prop-dict attr-dict
+}], [AttrSizedOperandSegments]>,
+ Arguments<(ins Variadic<I64>:$a1, Variadic<I64>:$a2)>;
+
+// When the format uses a bulk `operands`/`type(operands)` directive, the
+// segment sizes can't be reconstructed from the parse, so the
+// `operandSegmentSizes` key is required.
+// CHECK-LABEL: PropDictSegmentSizesRequired::setPropertiesFromParsedAttr
+// CHECK: auto operandSegmentSizesAttrName = ::mlir::StringAttr::get(ctx, "operandSegmentSizes");
+// CHECK-NEXT: usedKeys.insert(operandSegmentSizesAttrName);
+// CHECK-NEXT: auto attr = dict.get(operandSegmentSizesAttrName);
+// CHECK-NEXT: if (!attr && true) {
+// CHECK: if (attr && ::mlir::failed(::mlir::convertFromAttribute(prop.operandSegmentSizes, attr,
+def PropDictSegmentSizesRequired : TestFormat_Op<[{
+ `(` operands `)` `:` type(operands) prop-dict attr-dict
+}], [AttrSizedOperandSegments]>,
+ Arguments<(ins Variadic<I64>:$a1, Variadic<I64>:$a2)>;
+
// CHECK-LABEL: RegionRef::parse
// CHECK: auto odsResult = parseCustom(parser, *bodyRegion);
// CHECK-LABEL: RegionRef::print
>From 1f940458423cbea69d4f443aaf34e49c95fe415d Mon Sep 17 00:00:00 2001
From: Natanael Cintean <natanael.cintean at intel.com>
Date: Tue, 28 Jul 2026 11:59:56 +0000
Subject: [PATCH 3/4] Only insert the key in the bulk operand/results formater
case, i.e. when they can't be inferred.
---
mlir/test/IR/invalid-properties.mlir | 16 ++++++++--------
mlir/test/mlir-tblgen/op-format.td | 16 +++++++---------
mlir/tools/mlir-tblgen/OpFormatGen.cpp | 26 +++++++++++++++-----------
3 files changed, 30 insertions(+), 28 deletions(-)
diff --git a/mlir/test/IR/invalid-properties.mlir b/mlir/test/IR/invalid-properties.mlir
index 3678c98db43d1..27c5adbf78e69 100644
--- a/mlir/test/IR/invalid-properties.mlir
+++ b/mlir/test/IR/invalid-properties.mlir
@@ -43,20 +43,20 @@ func.func @wrong_dense_i32_array_prop_type() {
// -----
-// `operandSegmentSizes` is not required in `<{...}>` for this op, since its
-// format spells out each variadic group individually
-// If it is present anyway, it
-// must still be well-formed: it should not be silently ignored.
-func.func @malformed_optional_operand_segment_sizes(%arg0: i64) {
- // expected-error at +1 {{for `operandSegmentSizes`: expected DenseI32ArrayAttr}}
+// `operandSegmentSizes` is inferable from the individually-spelled-out
+// variadic groups in this op's format, so the custom parser never reads this
+// key at all: presence of the key is rejected as an unrecognized key, just
+// like any other name the op doesn't know about.
+func.func @unknown_optional_operand_segment_sizes(%arg0: i64) {
+ // expected-error at +1 {{unknown key '"operandSegmentSizes"' when parsing properties dictionary}}
test.variadic_segment_prop %arg0, %arg0 : %arg0 : i64, i64 : i64 <{operandSegmentSizes = "bad"}> end
return
}
// -----
-func.func @malformed_optional_result_segment_sizes(%arg0: i64) {
- // expected-error at +1 {{for `resultSegmentSizes`: expected DenseI32ArrayAttr}}
+func.func @unknown_optional_result_segment_sizes(%arg0: i64) {
+ // expected-error at +1 {{unknown key '"resultSegmentSizes"' when parsing properties dictionary}}
test.variadic_segment_prop %arg0, %arg0 : %arg0 : i64, i64 : i64 <{resultSegmentSizes = "bad"}> end
return
}
diff --git a/mlir/test/mlir-tblgen/op-format.td b/mlir/test/mlir-tblgen/op-format.td
index 7f388fd796f53..eea574888aee5 100644
--- a/mlir/test/mlir-tblgen/op-format.td
+++ b/mlir/test/mlir-tblgen/op-format.td
@@ -116,13 +116,11 @@ def OptionalGroupD : TestFormat_Op<[{
// When the format spells out each variadic operand group individually, the
// segment sizes can be inferred from what was actually parsed, so the
-// `operandSegmentSizes` key is optional (but still validated if present).
+// `operandSegmentSizes` key is left completely untouched: it isn't read,
+// validated, or marked as a used key at all.
// CHECK-LABEL: PropDictSegmentSizesInferred::setPropertiesFromParsedAttr
-// CHECK: auto operandSegmentSizesAttrName = ::mlir::StringAttr::get(ctx, "operandSegmentSizes");
-// CHECK-NEXT: usedKeys.insert(operandSegmentSizesAttrName);
-// CHECK-NEXT: auto attr = dict.get(operandSegmentSizesAttrName);
-// CHECK-NEXT: if (!attr && false) {
-// CHECK: if (attr && ::mlir::failed(::mlir::convertFromAttribute(prop.operandSegmentSizes, attr,
+// CHECK-NOT: operandSegmentSizes
+// CHECK: for (::mlir::NamedAttribute attr : dict) {
def PropDictSegmentSizesInferred : TestFormat_Op<[{
$a1 `:` $a2 prop-dict attr-dict
}], [AttrSizedOperandSegments]>,
@@ -130,13 +128,13 @@ def PropDictSegmentSizesInferred : TestFormat_Op<[{
// When the format uses a bulk `operands`/`type(operands)` directive, the
// segment sizes can't be reconstructed from the parse, so the
-// `operandSegmentSizes` key is required.
+// `operandSegmentSizes` key is required and validated.
// CHECK-LABEL: PropDictSegmentSizesRequired::setPropertiesFromParsedAttr
// CHECK: auto operandSegmentSizesAttrName = ::mlir::StringAttr::get(ctx, "operandSegmentSizes");
// CHECK-NEXT: usedKeys.insert(operandSegmentSizesAttrName);
// CHECK-NEXT: auto attr = dict.get(operandSegmentSizesAttrName);
-// CHECK-NEXT: if (!attr && true) {
-// CHECK: if (attr && ::mlir::failed(::mlir::convertFromAttribute(prop.operandSegmentSizes, attr,
+// CHECK-NEXT: if (!attr) {
+// CHECK: if (::mlir::failed(::mlir::convertFromAttribute(prop.operandSegmentSizes, attr,
def PropDictSegmentSizesRequired : TestFormat_Op<[{
`(` operands `)` `:` type(operands) prop-dict attr-dict
}], [AttrSizedOperandSegments]>,
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index f9d07f6b3ba0f..d1de4baa02e83 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1315,34 +1315,38 @@ ::mlir::MLIRContext *ctx = dict.getContext();
// `operandSegmentSizes`/`resultSegmentSizes` are trait-injected properties
// not enumerated by `op.getProperties()`, so they need to be special-cased
- // here, mirroring the handling in `setPropertiesFromAttr` in
- // OpDefinitionsGen.cpp.
+ // here, mirroring `setPropertiesFromAttr` in OpDefinitionsGen.cpp. This is
+ // only necessary when the format can't infer the sizes itself (bulk
+ // `operands`/`type(results)` directives); when the format spells out each
+ // variadic group individually, `genParserVariadicSegmentResolution` always
+ // overwrites the property from the parsed operand/result groups, so the key
+ // is left completely untouched here (same as any other property not
+ // handled by this format).
//
// {0}: segment sizes property name
- // {1}: isRequired
const char *segmentSizesFromAttrFmt = R"decl(
auto {0}AttrName = ::mlir::StringAttr::get(ctx, "{0}");
usedKeys.insert({0}AttrName);
auto attr = dict.get({0}AttrName);
-if (!attr && {1}) {{
+if (!attr) {{
emitError() << "expected key entry for {0} in DictionaryAttr to set "
"Properties.";
return ::mlir::failure();
}
-if (attr && ::mlir::failed(::mlir::convertFromAttribute(prop.{0}, attr, [&]() {{
+if (::mlir::failed(::mlir::convertFromAttribute(prop.{0}, attr, [&]() {{
return emitError() << "for `{0}`: ";
})))
return ::mlir::failure();
)decl";
- if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) {
+ if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments") &&
+ fmt.allOperands) {
auto scope = body.scope("{\n", "}\n", /*indent=*/true);
- body << formatv(segmentSizesFromAttrFmt, "operandSegmentSizes",
- fmt.allOperands);
+ body << formatv(segmentSizesFromAttrFmt, "operandSegmentSizes");
}
- if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments")) {
+ if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments") &&
+ fmt.allResultTypes) {
auto scope = body.scope("{\n", "}\n", /*indent=*/true);
- body << formatv(segmentSizesFromAttrFmt, "resultSegmentSizes",
- fmt.allResultTypes);
+ body << formatv(segmentSizesFromAttrFmt, "resultSegmentSizes");
}
// {0}: fromAttribute call
>From 1a0497ccb15b9efbe7a8a4aa94b11c49accf7f5d Mon Sep 17 00:00:00 2001
From: Natanael Cintean <natanael.cintean at intel.com>
Date: Tue, 28 Jul 2026 12:45:31 +0000
Subject: [PATCH 4/4] Handle no declarative format corner case.
---
mlir/tools/mlir-tblgen/OpFormatGen.cpp | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index d1de4baa02e83..4d5c31ae2ecae 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1317,7 +1317,10 @@ ::mlir::MLIRContext *ctx = dict.getContext();
// not enumerated by `op.getProperties()`, so they need to be special-cased
// here, mirroring `setPropertiesFromAttr` in OpDefinitionsGen.cpp. This is
// only necessary when the format can't infer the sizes itself (bulk
- // `operands`/`type(results)` directives); when the format spells out each
+ // `operands`/`type(results)` directives, or when there is no declarative
+ // assemblyFormat at all -- e.g. `hasCustomAssemblyFormat`, where this
+ // generated setter may still be reused by a hand-written parser that has
+ // no other way to recover the sizes); when the format spells out each
// variadic group individually, `genParserVariadicSegmentResolution` always
// overwrites the property from the parsed operand/result groups, so the key
// is left completely untouched here (same as any other property not
@@ -1338,13 +1341,14 @@ if (::mlir::failed(::mlir::convertFromAttribute(prop.{0}, attr, [&]() {{
})))
return ::mlir::failure();
)decl";
+ bool hasNoDeclarativeFormat = !op.hasAssemblyFormat();
if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments") &&
- fmt.allOperands) {
+ (hasNoDeclarativeFormat || fmt.allOperands)) {
auto scope = body.scope("{\n", "}\n", /*indent=*/true);
body << formatv(segmentSizesFromAttrFmt, "operandSegmentSizes");
}
if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments") &&
- fmt.allResultTypes) {
+ (hasNoDeclarativeFormat || fmt.allResultTypes)) {
auto scope = body.scope("{\n", "}\n", /*indent=*/true);
body << formatv(segmentSizesFromAttrFmt, "resultSegmentSizes");
}
More information about the Mlir-commits
mailing list