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

[Nagy Mostafa via Phabricator via llvm-commits]

nmostafa marked an inline comment as done.
nmostafa added a comment.

In D74174#1870307 <https://reviews.llvm.org/D74174#1870307>, @schweitz wrote:

> How difficult is it to support both the old-style "does not return a value" and the new terminator that does return a value?


Not difficult, but since the new YieldOp terminator optionally return a value, it can serve both purposes.



================
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);
----------------
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
}
```


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

https://reviews.llvm.org/D74174