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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:371
 
-  // Fully specified by traits.
-  let verifier = ?;
+  let printer = ?;
 }
----------------
nmostafa wrote:
> 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 ? 
Oops forgot about the variadic operands bit, sorry about that. You will need to use c++ for that for now.

> The original TerminatorOp didn't have a custom printer, so I just stuck with that.

That's because it was never printed. Now it can be, so it actually needs a printer. If you are defining a parser, you need a printer.


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

https://reviews.llvm.org/D74174