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

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer added a comment.

This looks like a really great start!  Thanks for sketching this out.



================
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:
> 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.


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

https://reviews.llvm.org/D74174