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

[Aart Bik via Phabricator via llvm-commits]

aartbik marked 4 inline comments as done.
aartbik added inline comments.


================
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);
----------------
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?


================
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());
 
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73593