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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

I am mostly nitpicking. Let's finish the design discussion on discourse and land this!



================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:152
+    /// Number of operands controlling the loop: lb, ub, step
+    constexpr unsigned getNumCtrlOperands() { return 3; }
+    /// Does the operation hold operands for loop-carried values
----------------
herhut wrote:
> Should this just be a `static constexpr unsigned kNumCtrlOperands`?
Nit: can we also spell out Control? It only needs 3 more characters.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:81
+      // iter_args binds initial values to the loop's region arguments.
+      %sum = loop.for %iv = %lb to %ub step %step iter_args(%sum_iter = %sum_0 : f32) -> (f32) {
+	      %t = load %buffer[%iv] : memref<1024xf32>
----------------
Random observation: we mention the same type twice in `iter_args` and in the trailing arrow-type, which are also located close. Did you consider omitting the trailing type?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:355
+def YieldOp : Loop_Op<"yield", [Terminator]> {
+  let summary = "cf yield and termination operation";
   let description = [{
----------------
Let's replace `cf` with `loop` here. `cf` dates back to a temporary state when the dialect used the prefix `cf` for "control flow".


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:360
+    is defined by the parent operation. 
+    If "loop.yield" has any operands, the operands must match the parent
+    operation's results. 
----------------
"the operands must match..." should `yield` enforce this or should the parent operations enforce this?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:362
+    operation's results. 
+    If the parent op defines no values, then the "loop.yield" can be 
+    left out and will be inserted implicitly. Otherwise, it has 
----------------
Nit: it is _always_ present, we just omit it while pretty printing. Could you please rephrase this text to reflect that fact? Otherwise, it creates an incentive to check for the presence when working with loop.if/for in the code.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:158-168
+  if (!parser.parseOptionalKeyword("iter_args")) {
+    parser.parseAssignmentList(regionArgs, operands, argTypes);
+    // resolve input operands
+    for (auto operand_type : llvm::zip(operands, argTypes))
+      parser.resolveOperand(std::get<0>(operand_type),std::get<1>(operand_type), result.operands);
+  }
+  argTypes.insert(argTypes.begin(), indexType);
----------------
nmostafa wrote:
> stephenneuendorffer wrote:
> > nmostafa wrote:
> > > rriddle wrote:
> > > > stephenneuendorffer wrote:
> > > > > For the for() loop syntax, I think that iter_args and the result type list should go together.  They are either both present or both absent.  As a result, there should be no need to replicate the type information 2x in the op syntax.
> > > > > 
> > > > > I think this could work two ways:
> > > > > 
> > > > > for()....iter_args(%x=%y) -> (i32) {...}
> > > > > if %c -> (i32) {...} else {...}  
> > > > > 
> > > > > However, with the else branch of the if being optional, it might make sense to have:
> > > > > 
> > > > > for()....args(%x=%y : i32) {...}
> > > > > if %c args(%x=%y: i32) {...} else {...}
> > > > > 
> > > > > This would also more conveniently represent the case where %x is assigned in one branch of the if, but not the other.
> > > > > 
> > > > > 
> > > > Don't use !, use succeeded/failed(...) instead when parsing optional constructs.
> > > I personally prefer seeing the types right next to the operands for better readability (especially if you have too many operands, you have to look at the end of the op to figure out the types). 
> > > 
> > > The result types are still convenient look at to quickly figure out the return types, but I am happy to remove it if there is consensus. 
> > @ftynse also suggested leaving off the return type for the for loop.  Anyone else have an opinion on this?  I'd really prefer for() and if() to be consistent in the respect, so I think in this incarnation, either the syntax for for() or if() needs to change.
> We still need the return type for the IfOp, not sure how omitting it for ForOp would help consistency. Maybe I am not sure I follow the IfOp format you are suggesting:
> ```if %c args(%x=%y: i32) {...} else {...}```
> 
> Unlike ForOp, there is no binding needed upon entering the IfOp regions. Can you re-write the example below using the format you are suggesting  ?  
> ```
> %d = if %c -> (f32) {  
>    %r1 = ... : f32
>     yield %r1 : f32
> } else {  
>    %r2 = ... : f32
>     yield %r2 : f32
> }
> ```
I don't have a strong opinion and I am receptive to the argument of consistency, i.e. `for` and `if` should have as close syntax as possible.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:92
+    return op.emitOpError(
+        "mismatch in number of op loop-carried values and defined values");
+  if (op.getNumRegionIterArgs() != opNumResults)
----------------
Nit: "op lop-carried" sounds weird


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:102
+      return op.emitOpError()
+             << "types mismatch between iter operands and defined values";
+    if (std::get<1>(e).getType() != std::get<2>(e).getType())
----------------
Mega-nit: I find it helpful to print the index of the mismatching type


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:127
+  }
+  if (!op.results().empty()) {
+    p << " -> (" << op.getResultTypes() << ")";
----------------
Doesn't the presence of iter operands imply there should be results?


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

https://reviews.llvm.org/D74174