[PATCH] D74174: [MLIR] Allow Loop dialect IfOp and ForOp to define values

[Nagy Mostafa via Phabricator via llvm-commits]

nmostafa marked 2 inline comments as done and an inline comment as not done.
nmostafa added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:371
 
-  // Fully specified by traits.
-  let verifier = ?;
+  let printer = ?;
 }
----------------
rriddle wrote:
> Why are you not specifying a printer? Also, can you just use the declarative form? That would remove the need for defining the parser/printer in c++.
> 
> https://mlir.llvm.org/docs/OpDefinitions#declarative-assembly-format
> 
> Should be something like:
> ```
> let assemblyFormat = "operands attr-dict `:` type(operands)";
> ```
The original TerminatorOp didn't have a custom printer, so I just stuck with that. 

If you feel strongly about it, I can change it. However, using `assemblyFormat` will yield an awkward printing when there are no operands: `loop.yield :` and parsing will fail if the colon is missing. Is there a way to specify optional tokens in the declarative form ? 


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:160
+  if (succeeded(parser.parseOptionalKeyword("iter_args"))) {
+    parser.parseAssignmentList(regionArgs, operands, argTypes);
+    // Resolve input operands.
----------------
rriddle wrote:
> This needs to be checked for failure, missing tests?
What needs to be checked ? `iter_args` in the syntax ? 


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

https://reviews.llvm.org/D74174