[PATCH] D133166: [mlir][MemRef] Canonicalize extract_strided_metadata(subview)

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:92
+        return getAsOpFoldResult(
+            rewriter.create<arith::ConstantIndexOp>(origLoc, altValue));
+      return opr;
----------------
nicolasvasilache wrote:
> You can just use `IntegerAttr Builder::getIndexAttr(int64_t value)` to avoid materializing the constant.
> This should work out of the box with OpFoldResult
Ah great!
Looking.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:112
+      strides.push_back(makeComposedFoldedAffineApply(
+          rewriter, origLoc, s0 * s1, {subStrides[i], getOrigStrideAtIdx(i)}));
+
----------------
nicolasvasilache wrote:
> This feels like too many levels of indirection for no good reason.
> How about something resembling:
> ```
> for () {
>   OpFoldResult origStride = (ShapedType::isDynamicStrideOrOffset(sourceStrides[i])) ? 
>     b.getIndexAttr(sourceStrides[i])  : origStrides[i];
>    strides.push_back(makeComposedFoldedAffineApply(
>        rewriter, origLoc, s0 * s1, {subStrides[i], origStride}));
> }
> ```
Sounds like I should merge the loop for the strides and the loop of the offset then.
Otherwise I will repeat in both loops:
```
  OpFoldResult origStride = (ShapedType::isDynamicStrideOrOffset(sourceStrides[i])) ? 
    b.getIndexAttr(sourceStrides[i])  : origStrides[i];
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166