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

[Adrian Kuegel via Phabricator via llvm-commits]

akuegel added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:163
+
+    ```mlir
+       loop.parallel (%iv) = (%lb) to (%ub) step (%step) {
----------------
nicolasvasilache wrote:
> Please add a few more examples that also illustrate reduction and the yield ops.
I extended the example so that it also has a loop.reduce inside.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:171
+  let arguments = (ins Variadic<Index>:$lowerBound,
+                   Variadic<Index>:$upperBound,
+		   Variadic<Index>:$step);
----------------
rriddle wrote:
> Can you format this a bit better?
> 
> let arguments = (ins Variadic...,
>                                    Variadic...,
>                                    Variadic...);
I hope I understood your suggestion correctly. I have now indented it such that the words Variadic align.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:198
 
+def YieldOp : Loop_Op<"yield", [HasParent<"ParallelOp">]> {
+  let summary = "yield operation for parallel for";
----------------
nicolasvasilache wrote:
> Do we foresee another use case for this than reduction?
> If not, does it make sense to call this a ReductionOp or a ReduceOp?
I renamed it to ReduceOp, and accordingly the other op to ReduceReturnOp instead of YieldReturnOp.
Also, the textual format is now loop.reduce and loop.reduce.return.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:227
+        ^bb0(%lhs : f32, %rhs: f32):
+	  loop.yield.return %rhs: f32
+      } : f32
----------------
nicolasvasilache wrote:
> rriddle wrote:
> > This looks like it needs to be formatted.
> can we make this example a bit more realistic?
> Unless I am misunderstanding, there should be a reduction operation in the region?
My editor decided it should add a tab when I hit carriage return. Visually it looked good in my editor, but of course I should use spaces here for indentation :)


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:234
+  let arguments = (ins AnyType:$operand);
+  let regions = (region SizedRegion<1>:$operation);
+}
----------------
rriddle wrote:
> 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.
What about reductionOperator?


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:343
+  auto type = op.operand()->getType();
+  auto &block = op.operation().front();
+  if (block.getNumArguments() != 2 ||
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72394