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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula accepted this revision.
bondhugula marked an inline comment as done.
bondhugula added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:371
 
-  // Fully specified by traits.
-  let verifier = ?;
+  let printer = ?;
 }
----------------
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!).




================
Comment at: mlir/test/Dialect/Loops/ops.mlir:112
+// CHECK-LABEL: func @std_if_yield(
+//  CHECK-SAME: %[[ARG0:[A-Za-z0-9]+]]:
+//  CHECK-SAME: %[[ARG1:[A-Za-z0-9]+]]:
----------------
extra space added here


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

https://reviews.llvm.org/D74174