[PATCH] D134826: [mlir][MemRef] Simplify extract_strided_metadata(collapse_shape)

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:181
+///
 /// \post result.size() == expandShape.getReassociationIndices()[groupId].size()
 ///
----------------
Note: I can split the commit with the NFC part first. This would be one of the changes in that commit.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:345
+                                ArrayRef<OpFoldResult> values,
+                                std::function<bool(int64_t)> isDynamic) {
+  AffineExpr productOfValues = builder.getAffineConstantExpr(1);
----------------
I thought I would need that more than once, but turned out I used it only once.
Let me know if you want that we get rid of it.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:466
 /// materialize its effects on the sizes and the strides using affine apply.
-struct ExtractStridedMetadataOpExpandShapeFolder
+template <typename ReshapeLikeOp,
+          SmallVector<OpFoldResult> (*getReshapedSizes)(
----------------
If you have a better name for this template type I'm taking!

In particular, this class is meant to deal with reshape ops with reassociation groups like collapse_shape and expand_shape. My problem with the name is that it could be confusing since we have `memref.reshape` and we don't support that here.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:473
+              ArrayRef<OpFoldResult> /*origStrides*/, unsigned /*groupId*/)>
+struct ExtractStridedMetadataOpReshapeFolder
     : public OpRewritePattern<memref::ExtractStridedMetadataOp> {
----------------
This is another change that could be part of an NFC commit first.
Essentially, we handle expand_shape and collapse_shape in the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134826