[PATCH] D75206: [MLIR] Add explicit initial values for loop.parallel op.

[Stephan Herhut via Phabricator via llvm-commits]

herhut accepted this revision.
herhut added a comment.
This revision is now accepted and ready to land.

Thanks. This looks great with some minor changes addressed.



================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:238
+    values as operands that represent the lower bounds, upper bounds, steps and
+    optional initial values, respectively. The operation defines a variadic
+    number of SSA values for its induction variables. It has one region
----------------
This confused me when I read it. The initial values are not optional. You need the same number of initial values as there are return values (which is mentioned below).


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:322
     bodyRegion->front().addArgument(builder->getIndexType());
-}
-
-void ParallelOp::build(Builder *builder, OperationState &result, ValueRange lbs,
-                       ValueRange ubs, ValueRange steps,
-                       ArrayRef<Type> resultTypes) {
-  result.addTypes(resultTypes);
-  build(builder, result, lbs, ubs, steps);
+  for (const auto &init : initVals) {
+    result.addTypes(init.getType());
----------------
Drop the `cons`t and `&` and avoid auto if the actual type is concise. `for (Value init : initVals)`.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:346
   if (body->getNumArguments() != stepValues.size())
-    return op.emitOpError(
-        "expects the same number of induction variables as bound and step "
-        "values");
+    return op.emitOpError("expects the same number of induction variables: ")
+           << body->getNumArguments()
----------------
I think it is preferred to not mix the two forms and just use `op.emitOpError() << ...`


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:428
+  int32_t numLoops = ivs.size();
+  result.addAttribute(
+      ParallelOp::getOperandSegmentSizeAttr(),
----------------
This should reflect what you actually parsed. So `lower.size()`, `upper.size()`, `steps.size()` and `initVals.size()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75206