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

Fri Feb 7 16:41:06 PST 2020

rriddle added inline comments.

================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:371

-  // Fully specified by traits.
-  let verifier = ?;
+  let printer = ?;
 }
----------------
bondhugula wrote:
> rriddle wrote:
> > 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.
> > The original TerminatorOp didn't have a custom printer, so I just stuc
> 
> Sounds good to me. The custom printer can be done in a separate patch. You can add a TODO - it would be good to have a custom print for this now that it's elided less (having quotes around it would look weird!).
> 
> 
The custom printer should be added in the same patch as the parser, whether it be this one or the next. I'm not comfortable with this going in with only one defined.

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

https://reviews.llvm.org/D74174