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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:171
+  let arguments = (ins Variadic<Index>:$lowerBound,
+                   Variadic<Index>:$upperBound,
+		   Variadic<Index>:$step);
----------------
Can you format this a bit better?

let arguments = (ins Variadic...,
                                   Variadic...,
                                   Variadic...);


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:227
+        ^bb0(%lhs : f32, %rhs: f32):
+	  loop.yield.return %rhs: f32
+      } : f32
----------------
This looks like it needs to be formatted.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:234
+  let arguments = (ins AnyType:$operand);
+  let regions = (region SizedRegion<1>:$operation);
+}
----------------
nit: Can we name this something other than 'operation'? Seems like this is likely to lead to confusion in the future. e.g. `computation` or `body` would achieve the same effect.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:241
+  if (stepValues.empty())
+    return op.emitOpError(
+        "need at least one tuple element for lowerBound, upperBound and step");
----------------
I would imagine that we would check this before the above loop.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:263
+  // Check that the types of the results and yields are the same.
+  for (auto result_and_yield : llvm::zip(op.results(), yields)) {
+    auto resultType = std::get<0>(result_and_yield)->getType();
----------------
Use camelCase for variable names.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:342
+  // The region of a YieldOp has two arguments of the same type as its operand.
+  auto type = op.operand()->getType();
+  auto &block = op.operation().front();
----------------
Drop the -> here.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:343
+  auto type = op.operand()->getType();
+  auto &block = op.operation().front();
+  if (block.getNumArguments() != 2 ||
----------------
I don't see a check for empty block, can you add one.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:384
+  p.printRegion(op.operation());
+  p << " : " << op.operand()->getType();
+}
----------------
Drop -> here. Same in a few other places below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72394