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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:118
+  let results = (outs Variadic<AnyType>:$results);
+  let arguments = (ins Index:$lowerBound, Index:$upperBound, Index:$step, Variadic<AnyType>:$initArgs);
   let regions = (region SizedRegion<1>:$region);
----------------
Is this wrapped at 80 characters?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:139
+    iterator_range<Operation::operand_iterator> getIterOperands() {
+      auto operands = getOperation()->getOperands();
+      auto iterOpndBegin = operands.begin();
----------------
Can you just do:

getOperands().drop_front(getNumCtrlOperands())

?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:149
+    
+    // number of region arguments for loop-carried values
+    unsigned getNumRegionIterArgs() { return getBody()->getNumArguments() - 1; }
----------------
Capitalize sentences and change these to ///


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:156
+    // get Number of loop-carried values
+    unsigned getNumIterOperands() { return getOperation()->getNumOperands() - getNumCtrlOperands(); }
   }];
----------------
This doesn't look like it's wrapped at 80 characters/


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:193
+    defines no values, the "loop.yield" can be left out, and will be
+    inserted implicitly. Otherwise, it must be explicitly. A
+    Also, if "loop.if" defines no values, the 'else' block is optional, and may be omitted. 
----------------
`A Also`?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:194
+    inserted implicitly. Otherwise, it must be explicitly. A
+    Also, if "loop.if" defines no values, the 'else' block is optional, and may be omitted. 
 
----------------
Same here.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:203
   }];
+  let results = (outs Variadic<AnyType>:$results);
   let arguments = (ins I1:$condition);
----------------
nit: arguments before results


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:354
+    is defined by the parent operation. 
+    If "loop.yield" has any operands, then parent op must define the same
+    number and types of the operands. 
----------------
then -> the


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:89
+    if (op.getNumIterOperands() != opNumResults)
+      op.emitOpError(
+          "mismatch in number of op loop-carried values and defined values");
----------------
You need to return when you emit an error.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:158
+
+  if (!parser.parseOptionalKeyword("iter_args")) {
+    parser.parseAssignmentList(regionArgs, operands, argTypes);
----------------
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.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:509
+//===----------------------------------------------------------------------===//
+void YieldOp::build(Builder *builder, OperationState &result) {}
+
----------------
Why is this necessary?


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:518
+      !isa<ParallelOp>(parentOp))
+    return op.emitError() << "yield terminate If, For or Parallel regions";
+
----------------
terminates


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:526
+      if (std::get<0>(e).getType() != std::get<1>(e).getType())
+        op.emitOpError() << "types mismatch between yield op and its parent";
+    }
----------------
Return on error.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:536
+  llvm::SMLoc loc = parser.getCurrentLocation();
+  // parser variadic opreands list, their types, and resolve operands to SSA
+  // values
----------------
Start sentences with capital letters. Also typo `opreands`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74174