[Mlir-commits] [mlir] [MLIR] Validate directly parsed optional operand types (PR #212493)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: lorenzo chelini (chelini)

<details>
<summary>Changes</summary>

Commit 6c51938067fa ([MLIR] Guard optional operand resolution in generated op parsers) guarded resolution of every optional operand to avoid indexing an empty inferred type range. The guard also skipped resolveOperands when an optional operand was absent but an independently optional type directive was present. This accepted malformed assembly and silently discarded the orphan type when printing the operation.

Keep the guard for inferred type resolutions, which may access an empty optional type vector, but always resolve directly parsed type ranges so resolveOperands validates operand/type cardinality. Add coverage for both valid forms and a type-only negative reproducer.

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


4 Files Affected:

- (modified) mlir/test/IR/invalid-ops.mlir (+5) 
- (modified) mlir/test/lib/Dialect/Test/TestOpsSyntax.td (+10) 
- (modified) mlir/test/mlir-tblgen/op-format.mlir (+6) 
- (modified) mlir/tools/mlir-tblgen/OpFormatGen.cpp (+12-5) 


``````````diff
diff --git a/mlir/test/IR/invalid-ops.mlir b/mlir/test/IR/invalid-ops.mlir
index 2f5dd28b51911..8f91d4e1d4c70 100644
--- a/mlir/test/IR/invalid-ops.mlir
+++ b/mlir/test/IR/invalid-ops.mlir
@@ -126,6 +126,11 @@ test.variadic_args_types_split "hello_world" : i32
 
 // -----
 
+// expected-error at +1 {{number of operands and types do not match: got 0 operands and 1 types}}
+test.format_optional_operand_type : i64
+
+// -----
+
 // Test multiple verifier errors in the same split to ensure all are reported.
 
 func.func @verify_fail_1() {
diff --git a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
index 096d4d255b729..1d9ff9bdef3a2 100644
--- a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
+++ b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
@@ -706,6 +706,16 @@ def FormatTypesMatchOptionalOp : TEST_Op<"format_types_match_optional", [
   }];
 }
 
+def FormatOptionalOperandTypeOp
+    : TEST_Op<"format_optional_operand_type"> {
+  let arguments = (ins Optional<AnyType>:$optional);
+  let assemblyFormat = [{
+    (`(` $optional^ `)`)?
+    (`:` type($optional)^)?
+    attr-dict
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // InferTypeOpInterface type inference in assembly format
 //===----------------------------------------------------------------------===//
diff --git a/mlir/test/mlir-tblgen/op-format.mlir b/mlir/test/mlir-tblgen/op-format.mlir
index 7ff9091d5500d..5f44e5cffe675 100644
--- a/mlir/test/mlir-tblgen/op-format.mlir
+++ b/mlir/test/mlir-tblgen/op-format.mlir
@@ -494,6 +494,12 @@ test.format_infer_variadic_type_from_non_variadic %i64, %i64 : i64
 // CHECK: test.format_types_match_optional
 test.format_types_match_optional
 
+// CHECK: test.format_optional_operand_type
+test.format_optional_operand_type
+
+// CHECK: test.format_optional_operand_type(%[[I64]]) : i64
+test.format_optional_operand_type(%i64) : i64
+
 //===----------------------------------------------------------------------===//
 // InferTypeOpInterface type inference
 //===----------------------------------------------------------------------===//
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index 4d5c31ae2ecae..2ae276a62e939 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -318,6 +318,10 @@ struct OperationFormat {
     std::optional<StringRef> getVarTransformer() const {
       return variableTransformer;
     }
+    /// Returns true if the type is parsed directly from the assembly format.
+    bool isDirectlyParsed() const {
+      return !builderIdx && !getVariable() && !getAttribute();
+    }
     void setResolver(ConstArgument arg, std::optional<StringRef> transformer) {
       resolver = arg;
       variableTransformer = transformer;
@@ -1890,19 +1894,22 @@ void OperationFormat::genParserOperandTypeResolution(
   // separately.
   for (unsigned i = 0, e = op.getNumOperands(); i != e; ++i) {
     NamedTypeConstraint &operand = op.getOperand(i);
-    // Optional operands may not be present; guard resolution to avoid
-    // out-of-bounds access on the (potentially empty) types vector.
-    if (operand.isOptional())
+    TypeResolution &operandType = operandTypes[i];
+    // Inferred type resolution may access another optional variable's empty
+    // type vector. Directly parsed type ranges are safe and must always be
+    // resolved so that operand/type cardinality is validated.
+    bool guardOptionalOperand =
+        operand.isOptional() && !operandType.isDirectlyParsed();
+    if (guardOptionalOperand)
       body << "  if (!" << operand.name << "Operands.empty()) {\n";
     body << "  if (parser.resolveOperands(" << operand.name << "Operands, ";
 
     // Resolve the type of this operand.
-    TypeResolution &operandType = operandTypes[i];
     emitTypeResolver(operandType, operand.name);
 
     body << ", " << operand.name
          << "OperandsLoc, result.operands))\n    return ::mlir::failure();\n";
-    if (operand.isOptional())
+    if (guardOptionalOperand)
       body << "  }\n";
   }
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/212493