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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked 8 inline comments as done.
qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:112
+      strides.push_back(makeComposedFoldedAffineApply(
+          rewriter, origLoc, s0 * s1, {subStrides[i], getOrigStrideAtIdx(i)}));
+
----------------
qcolombet wrote:
> 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];
> ```
@nicolasvasilache I'm going to leave the PR open until tomorrow to give you a chance to see the merged loops.
(In other words, what I did to avoid the duplication of the ternary op for the original stride after getting rid of the lambda.)


================
Comment at: mlir/test/Dialect/MemRef/simplify-extract-strided-metadata.mlir:103
+// => New strides: [64, 4, 1]
+// Final stides == filterOutReducedDim(new strides, 0) == [4 , 1]
+//
----------------
nicolasvasilache wrote:
> typo
Nice catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166