[PATCH] D79688: [mlir] loop::ForOp: provide builders with callbacks for loop body

[Alex Zinenko via Phabricator via llvm-commits]

ftynse marked 8 inline comments as done.
ftynse added a comment.

> On another note, I've never seen ValueVector before - I don't find it upstream either.

It's defined in line 47 of LoopOps.h / SCF.h in this commit.



================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.h:72
+
+/// A convenience version for building loop nests without reductions. Does not
+/// take the initial value of reductions or expect the body building functions
----------------
bondhugula wrote:
> Did you mean "without iteration arguments (like for reductions)" instead of "without reductions"?
I think one implies the other, but okay to change. 


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:496
   Value vec = vector_type_cast(tmp);
-  SmallVector<Value, 8> ivs(lbs.size());
-  LoopNestBuilder(ivs, lbs, ubs, steps)([&] {
+  loopNestBuilder(lbs, ubs, steps, [&](ValueRange loopIvs) {
+    auto ivs = llvm::to_vector<8>(loopIvs);
----------------
bondhugula wrote:
> /*bodyBuilder=*/
We use argument-naming comments for literals when they can be interpreted ambiguously https://llvm.org/docs/CodingStandards.html#comment-formatting. (e.g. we don't add comment to `getRegion(0)`.)  This argument is neither a literal nor really ambiguous. 


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:570
   std_store(vectorValue, vec);
-  SmallVector<Value, 8> ivs(lbs.size());
-  LoopNestBuilder(ivs, lbs, ubs, steps)([&] {
+  loopNestBuilder(lbs, ubs, steps, [&](ValueRange loopIvs) {
+    auto ivs = llvm::to_vector<8>(loopIvs);
----------------
bondhugula wrote:
> /*bodyBuilder=*/
Same as above, this defies the purpose of loop nest constructor.


================
Comment at: mlir/test/EDSC/builder-api-test.cpp:145
   using namespace edsc::op;
-  LoopNestBuilder(&i, a - b, c + d, a)();
+  loopNestBuilder(a - b, c + d, a);
 
----------------
bondhugula wrote:
> /*lbs=*/..., /*ubs=*/..., /*step=*/...)
Same as above, we don't systematically do this and I don't see a strong reason for it, especially in this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79688