[PATCH] D73593: [mlir] [VectorOps] consolidate all vector utilities to one header/cc file

[Diego Caballero via Phabricator via llvm-commits]

dcaballe accepted this revision.
dcaballe added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorUtils.h:37
+/// de-linearized index.
+SmallVector<int64_t, 4> delinearize(ArrayRef<int64_t> sliceStrides,
+                                    int64_t linearIndex);
----------------
aartbik wrote:
> dcaballe wrote:
> > All these utilities returning `SmallVector` should be turned into `void utility_name(..., SmallVectorImpl<element_type>& output_name)` so that they can accept SmallVectors of arbitrary static sizes.
> Note that this is a refactoring of existing code, so I would prefer to do that as follow up. But just in case you insist, can you point me to at least one occurrence of this elsewhere in the tree?
Agree, we can address this separately.
There are a few examples here: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Analysis/LoopInfo.h#L256


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:115
+  assert(static_cast<int64_t>(sizes.size()) == vectorType.getRank());
+  assert(static_cast<int64_t>(strides.size()) == vectorType.getRank());
 
----------------
aartbik wrote:
> dcaballe wrote:
> > just curious... why is this static_cast needed?
> size_t vs int64_t (may cause some compilers to complain about sign diff in comparison)
Thanks! Sorry, I thought the cast applied to the result of the comparison. My bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73593