[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/lib/Dialect/LoopOps/LoopOps.cpp:333
+  if (!op.results().empty()) {
+    p << " : ";
+    interleaveComma(op.getResultTypes(), p);
----------------
rriddle wrote:
> Stream in the results directly:
> << op.getResultTypes()
nit: Remove trivial braces


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:343
+  auto type = op.operand()->getType();
+  auto &block = op.operation().front();
+  if (block.getNumArguments() != 2 ||
----------------
akuegel wrote:
> rriddle wrote:
> > I don't see a check for empty block, can you add one.
> I think this is automatically checked because it is defined as a SizedRegion. See the test yield_no_block in invalid.mlir
> But as far as I understand Nicolas' comment, I actually should remove that test, too (leaving it here for now for verification if maybe I misunderstood something).
SizedRegion means the number of blocks, it doesn't constrain the number of operations within the blocks. e.g. an empty block would crash the isa<ReduceReturnOp> check below.


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:359
+static ParseResult parseReduceOp(OpAsmParser &parser, OperationState &result) {
+
+  // Parse an opening `(` followed by the reduced value followed by `)`
----------------
Still seems like there is a newline here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72394