[PATCH] D72394: [mlir] Add skeleton implementation of loop.parallel, loop.yield and loop.yield.return operations.

[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:162
+
+  let arguments = (ins Variadic<Index>:$lowerBound, Variadic<Index>:$upperBound, Variadic<Index>:$step);
+  let results = (outs Variadic<AnyType>:$results);
----------------
These should all be wrapped at 80 characters.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:167
+
+def YieldOp : Loop_Op<"yield", [HasParent<"ParallelOp">]> {
+  let summary = "yield operation for parallel for";
----------------
Keep these operations sorted alphabetically.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:205
+
+      loop.yield.return %res : f32
+  }];
----------------
Use:

```mlir

```


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:232
+  Operation::operand_range stepValues = op.step();
+  for (const auto &stepValue : stepValues)
+    if (auto cst =
----------------
Drop the `const &`

nit: auto -> Value


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:234
+    if (auto cst =
+            dyn_cast_or_null<ConstantIndexOp>(stepValue->getDefiningOp()))
+      if (cst.getValue() <= 0)
----------------
Don't use -> or * with values.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:247
+  // number of tuple elements in step.
+  auto *body = &op.body().front();
+  if (body->getNumArguments() != stepValues.size())
----------------
nit: auto -> Block


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:252
+        "values");
+  for (auto &arg : body->getArguments())
+    if (!arg->getType().isIndex())
----------------
Drop the &.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:253
+  for (auto &arg : body->getArguments())
+    if (!arg->getType().isIndex())
+      return op.emitOpError(
----------------
Same here, drop all of the -> and *


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:333
+  if (!op.results().empty()) {
+    p << " : ";
+    interleaveComma(op.getResultTypes(), p);
----------------
Stream in the results directly:
<< op.getResultTypes()


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:363
+
+  // Parse an opening `(` followed by the yielded value followed by `)`
+  OpAsmParser::OperandType operand;
----------------
Drop extra newline


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:417
+  p << op.getOperationName() << " ";
+  p.printOperand(op.result());
+  p << " : ";
----------------
Stream the result and the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72394