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

[Andy Davis via Phabricator via llvm-commits]

andydavis1 accepted this revision.
andydavis1 added a comment.
This revision is now accepted and ready to land.

Great cleanup. The call sites look great. Thanks for doing this!



================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorUtils.h:31
+/// for each dimension.
+SmallVector<int64_t, 4> makeSliceStrides(ArrayRef<int64_t> shape,
+                                         ArrayRef<int64_t> sizes);
----------------
I think this could be named "computeStrides" as it could be applied to non-slices as well. It just takes a 'shape' and 'sizes'.


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorUtils.h:40
+
+/// Given the sizes of a vector, together with vector-space offsets, returns
+/// the element-space offsets for each dimension.
----------------
Since this is a library it might be good to make the name more verbose:

computeElementOffsetsFromVectorSliceOffsets


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorUtils.h:40
+
+/// Given the sizes of a vector, together with vector-space offsets, returns
+/// the element-space offsets for each dimension.
----------------
andydavis1 wrote:
> Since this is a library it might be good to make the name more verbose:
> 
> computeElementOffsetsFromVectorSliceOffsets
Given the target slice sizes of a vector...


================
Comment at: mlir/include/mlir/Dialect/VectorOps/VectorUtils.h:47
+/// the slize sizes for each dimension.
+SmallVector<int64_t, 4> makeSliceSizes(ArrayRef<int64_t> shape,
+                                       ArrayRef<int64_t> sizes,
----------------
nit: I prefer "computeSliceSizes", but good either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73593