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

Alex Zinenko via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 12 04:47:57 PDT 2020


ftynse marked an inline comment as done.
ftynse 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);
----------------
bondhugula wrote:
> 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. 
I linked the only relevant piece I found.

Please prefer discourse (forum) to discord (chat), I find it extremely annoying to use the chat because it prompts short immediate answers that must arrive during night time at my location, or are otherwise ignored due to the lack of context. I will spare my arguments for that discussion. I don't think this change needs to be blocked on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79688





More information about the llvm-commits mailing list