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

[Nagy Mostafa via Phabricator via llvm-commits]

nmostafa marked 3 inline comments as done.
nmostafa added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:126
   let builders = [
     OpBuilder<"Builder *builder, OperationState &result, "
               "Value lowerBound, Value upperBound, Value step">
----------------
herhut wrote:
> Should we add a builder with support for iter_args as well?
To build an empty loop with no updates to the init args ? Otherwise, the loop will be invalid if the YieldOp has no operands. 
Seems a bit awkward to me as opposed to generating an empty loop with no variables and have the user add variables to the ForOp, Region and YieldOp. 


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:371
 
-  // Fully specified by traits.
-  let verifier = ?;
+  let printer = ?;
 }
----------------
bondhugula wrote:
> rriddle wrote:
> > 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.
> Okay. In that case, the commit message should be updated to reflect that (addition of custom parser/printer for yield).
Actually, the TerminatorOp was printed before with the default printer. 
ops.mlir: `// CHECK:   "loop.terminator"() : () -> ()`

The only difference is that yield now can be explicit in the source and that's why a custom parser makes sense. 
Let me add a custom printer and change the tests. 


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

https://reviews.llvm.org/D74174