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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked an inline comment as done.
bondhugula added inline comments.


================
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);
----------------
ftynse wrote:
> 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. 
That URL is about comment formatting rules, and not about the necessity of such argument documentation. The LLVM style is actually silent on recommending its usage except a passing reference there, and so is the MLIR style. I think we should discuss this separately on discord --- using such documenting names where it's not obvious what the argument is (to a new user/browser) greatly improves call site readability, and in this instance as well. Unfortunately, a lot of the new code is missing out on this due to the lack of a guideline. Fine for this commit, but I'll bring this up for discussion on the list. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79688