[PATCH] D102679: [llvm] Make Sequence reverse-iterable

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 8 10:54:03 PDT 2021


mehdi_amini added inline comments.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:321
-  SmallVector<int64_t> transposition(
-      llvm::seq<int64_t>(0, linalgOp.getNumLoops()));
   std::swap(transposition.back(), transposition[indexOp.dim()]);
----------------
Quuxplusone wrote:
> mehdi_amini wrote:
> > Why did we lose this direct initialization pattern? Can we adjust something to keep this?
> FWIW, I don't think we //want// to keep this. A lazy `llvm::seq` object physically-is something very different from a `SmallVector<int64_t>`; I think it's very good that the new code separates them. It might be even better to just use the `std::iota` algorithm to initialize the vector's contents:
> ```
> SmallVector<int64_t> transposition(linalgOp.getNumLoops());
> std::iota(transposition.begin(), transposition.end(), 0);
> std::swap(transposition.back(), transposition[indexOp.dim()]);
> ```
> If we were to keep it a one-liner, I would argue in favor of a named method on `seq`:
> ```
> SmallVector<int64_t> transposition =
>     llvm::seq<int64_t>(0, linalgOp.getNumLoops()).asSmallVector();
> std::swap(transposition.back(), transposition[indexOp.dim()]);
> ```
>  A lazy llvm::seq object physically-is something very different from a SmallVector<int64_t>

Yes of course, but I don't see the point you're making here? I'm getting a lazy-range thing and I want to materialize it in a vector.   We wouldn't want an implicit conversion, but that's not a reason to disallow the explicit materialization.

The question is what is the most concise and readable code for this (one-liner if possible), an llvm::iota may be palatable (to avoid begin/end forced by the std:: version), but I still find it adding a cognitive load: it actually requires to initialize a vector of zeroes before overwriting it with iota.

Probably not enough of a common pattern to optimize for though...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102679



More information about the llvm-commits mailing list