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

[Nicolas Vasilache via Phabricator via llvm-commits]

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

Looks good, thanks!



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


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:112
+      strides.push_back(makeComposedFoldedAffineApply(
+          rewriter, origLoc, s0 * s1, {subStrides[i], getOrigStrideAtIdx(i)}));
+
----------------
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}));
}
```


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:120
+    AffineExpr expr = symbols.front();
+    values[0] = getAlternativeValue(
+        getAsOpFoldResult(newExtractStridedMetadata.getOffset()), sourceOffset);
----------------
same here, just spell it out with a ternary cond, the level of indirection dos not pay for itself.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:144
+    unsigned subRank = subType.getRank();
+    // Properly size the arrray so that we can do random insertions
+    // at the right indices.
----------------
nit: arrray



================
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]
+//
----------------
typo


================
Comment at: mlir/test/Dialect/MemRef/simplify-extract-strided-metadata.mlir:151
+// => New strides: [64, 4 * %stride, 1]
+// Final stides == filterOutReducedDim(new strides, 0) == [4 * %stride , 1]
+//
----------------
typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166