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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:729
+static SmallVector<int64_t, 4> getI64SubArray(ArrayAttr arrayAttr,
+                                              unsigned dropFront = 1,
+                                              unsigned dropBack = 0) {
----------------
Why default `1` here, it's not obvious to me that `getI64SubArray(attr)` will drop the first element.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:733
+  SmallVector<int64_t, 4> res;
+  res.reserve(arrayAttr.size() - dropFront - dropBack);
+  for (auto it = range.begin() + dropFront, eit = range.end() - dropBack;
----------------
You need a range check, otherwise you risk reserving huge spaces when you drop more elements that you have due to unsigned arithmetic.


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:736
+       it != eit; ++it)
+    res.push_back((*it).getValue().getSExtValue());
+  return res;
----------------
Would `it->getValue()` work ?


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:740
+
+static Value extractOne(PatternRewriter &rewriter, Location loc, Value vector,
+                        int64_t offset) {
----------------
Please document this


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:803
+        if (!matchAndRewrite(stridedSliceOp, rewriter))
+          return matchFailure();
+        extracted = stridedSliceOp;
----------------
What will happen to the operations we already inserted or deleted if we fail after modifying the IR? Have you tried it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72310