[PATCH] D77863: [mlir][ODS] Add support for optional operands and results with a new Optional directive.

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.
This revision is now accepted and ready to land.

Nice! Thnks River!



================
Comment at: mlir/docs/OpDefinitions.md:225
+variadic operand definition. Though, if an operation has more than one variable
+length operands(either optional or variadic), it would be impossible to
+attribute dynamic operands to the corresponding static variadic operand
----------------
nit: space before (


================
Comment at: mlir/docs/OpDefinitions.md:228
+definitions without further information from the operation. Therefore, the
+`SameVariadicOperandSize` trait is needed to indicate that all variable length
+operands have the same number of dynamic values.
----------------
Also mention AttrSizedOperandSegments?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1103
+           << "    " << builderOpState << ".addOperands(" << argName << ");\n";
+      continue;
+    }
----------------
Nit: `else` would be better here. We don't have a large body of code following this anyway.


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:488
+  Optional,
+  /// The argument is a single element, i.e. always represents 1 element.s
+  Single
----------------
remove trailing s


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77863/new/

https://reviews.llvm.org/D77863