[PATCH] D78093: [mlir] [EDSC] Add interface for yield-for loops.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

I have two concerns:

- this does not seem to work with builders that have the insertion point in the middle of a block;
- it is unclear if this is intended to work for loop _nests_.



================
Comment at: mlir/include/mlir/Dialect/LoopOps/EDSC/Builders.h:38
+                            ArrayRef<ValueHandle *> iter_args_handles = {},
+                            ArrayRef<Value> iter_args_init_values = {});
 
----------------
Nit: ValueRange instead of `ArrayRef<Value>` unless you have a strong reason to do otherwise.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/EDSC/Builders.h:59
 public:
+  LoopNestBuilder(edsc::ValueHandle *iv, ValueHandle lb, ValueHandle ub,
+                  ValueHandle step,
----------------
Nit: no need to prefix with `edsc::` if you are already inside the `edsc` namespace


================
Comment at: mlir/include/mlir/Dialect/LoopOps/EDSC/Intrinsics.h:2
+//===- Intrinsics.h - MLIR EDSC Intrinsics for Linalg -----------*- C++ -*-===//
+////
+//// Part of the LLVM Project, under the Apache License v2.0 with LLVM
----------------
Spurious leading `//` makes you have `////`


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:63
+                                     iter_args_init_values));
+  assert(loops.size() == 1 && "Mismatch loops vs ivs size");
+}
----------------
I don't understand this assert, and the message does not really help.  Do you only support building one loop with iter args?  If so, have you considered modifying `LoopBuilder` instead of `LoopNestBuilder` ?


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:77
+      ScopedContext::getBuilder().getInsertionBlock()->getOperations();
+  // The for loop was the last op to be inserted in this block.
+  Operation &forloopop = operations.back();
----------------
While it was the last to be inserted, nothing guarantees that it's the last operation in the block. The insertion point may have been pointing in the middle of the block.  I would suggest to just store the results somewhere when you create the loop operation and return them here....


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:78
+  // The for loop was the last op to be inserted in this block.
+  Operation &forloopop = operations.back();
+  // Returns the results of the forloopop.
----------------
Nit: let's add some camelBack here: `forLoopOp`


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:111
   auto *body = loop::getForInductionVarOwner(iv->getValue()).getBody();
+  for (size_t i = 0; i < iter_args_handles.size(); ++i) {
+    // Skipping the induction variable.
----------------
Nit: `size_t i = 0, e = iter_args_handles.size(); i < e; ++i` to avoid calling `size` on every iteration. This idiom is extremely common in LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78093