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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added a comment.

Thanks! This looks great. I had written some comments but forgot to hit the submit button it seems :(



================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:126
   let builders = [
     OpBuilder<"Builder *builder, OperationState &result, "
               "Value lowerBound, Value upperBound, Value step">
----------------
Should we add a builder with support for iter_args as well?


================
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
----------------
Should this just be a `static constexpr unsigned kNumCtrlOperands`?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:259
     The body region must contain exactly one block that terminates with
-    "loop.terminator". Parsing ParallelOp will create such region and insert the
+    "loop.yield". Parsing ParallelOp will create such region and insert the
     terminator when it is absent from the custom format. For example:
----------------
bondhugula wrote:
> such a region
Mention that the `yield` may not have operands.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:353
 
-def TerminatorOp : Loop_Op<"terminator", [Terminator]> {
-  let summary = "cf terminator operation";
+def YieldOp : Loop_Op<"yield", [Terminator]> {
+  let summary = "cf yield and termination operation";
----------------
I wonder whether we should keep the TerminatorOp for the `loop.parallel`. It is always implicit there anyway and has different semantics (the return values of `loop.parallel` are defined by `loop.reduce` operation. Having the extra op is not costing much and avoid the extra checks in verifiers.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:522
+
+  if (isa<IfOp>(parentOp) || isa<ForOp>(parentOp)) {
+    if (parentOp->getNumResults() != op.getNumOperands())
----------------
Could you also verify that in the case of a Parallel operation the yield does not have any operands?


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

https://reviews.llvm.org/D74174