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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:63
+                                     iter_args_init_values));
+  assert(loops.size() == 1 && "Mismatch loops vs ivs size");
+}
----------------
ftynse wrote:
> 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` ?
+1, you already modify the static makeLoopBuilder method inside LoopBuilder.
It seems LoopBuilder is the natural thing to extend and we can revisit LoopNestBuilder later if/when necessary.


================
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();
----------------
ftynse wrote:
> 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....
Does this just go away if we focus on just LoopBuilder?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78093