[PATCH] D74288: [MLIR][Affine] Add affine.parallel op

[River Riddle via Phabricator via llvm-commits]

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

Looks great, thanks!



================
Comment at: mlir/include/mlir/Dialect/AffineOps/AffineOps.td:287
+    The body region must contain exactly one block that terminates with
+    "affine.terminator". Calling AffineParallelOp::build will create the
+    required region and block, and insert the required terminator. Parsing will
----------------
nit: Can you move `Calling AffineParallelOp::...` down to a `Note:` section?


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2209
+  bodyRegion->push_back(body);
+  ensureTerminator(*bodyRegion, *builder, result.location);
+}
----------------
flaub wrote:
> `ensureTermniator` is unnecessary, or just the redundant comment line?
The comment, sorry for the confusion.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2215
+AffineParallelOp::operand_range AffineParallelOp::getLowerBoundsOperands() {
+  return {operand_begin(), operand_begin() + lowerBoundsMap().getNumInputs()};
+}
----------------
nit: you could do `getOperands().take_front(lowerBoundsMap().getNumInputs())`


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2219
+AffineParallelOp::operand_range AffineParallelOp::getUpperBoundsOperands() {
+  return {operand_begin() + lowerBoundsMap().getNumInputs(), operand_end()};
+}
----------------
nit: Similarly here, but with drop_front.


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2275
+  p << op.getOperationName() << " (";
+  p.printOperands(op.getBody()->getArguments());
+  p << ") = (";
----------------
nit: Can you stream the arguments?


================
Comment at: mlir/lib/Dialect/AffineOps/AffineOps.cpp:2372
+  AffineParallelOp::ensureTerminator(*body, builder, result.location);
+
+  return success();
----------------
nit: Remove the newline here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74288