[PATCH] D72310: [mlir][VectorOps] Implement strided_slice conversion

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/IR/Attributes.h:1453
+  using reference = UnderlyingTy;
+  explicit attr_value_iterator(ArrayAttr::iterator it)
+      : llvm::mapped_iterator<ArrayAttr::iterator, UnderlyingTy (*)(Attribute)>(
----------------
rriddle wrote:
> rriddle wrote:
> > This needs more work. Will try to respond by end of day.
> (sorry for the delay)
> 
> I would move the iterator, and anything related, to ArrayAttr itself. This is cluttering the global `mlir` namespace, which is something that we shouldn't do in general. I would imagine that we would have some 'get*<>' method on ArrayAttr that would hide this from the user(similarly to how we do for DenseElementsAttr). For the default behavior, many attributes already have a 'getValue' method which could be used directly. Otherwise, we could just return the derived attribute itself. 
getValue returns APInt so that does not work out of the box.
Returning an iterator over just the cast attribute to the proper type which simplifies the API.
Unpacking into the raw underlying type in the client where I have to materialize anyway.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:737
+
+static Value extractOne(PatternRewriter &rewriter, Location loc, Value vector,
+                        int64_t offset) {
----------------
andydavis1 wrote:
> These will be merged with other CL, yes?
yes, the other CL rebases on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72310