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

[Quentin Colombet via Phabricator via llvm-commits]

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


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:342
+/// \pre for all index in indices: index < maybeConstants.size()
+OpFoldResult getProductOfValues(ArrayRef<int64_t> indices, OpBuilder &builder,
+                                Location loc, ArrayRef<int64_t> maybeConstants,
----------------
chelini wrote:
> static ?
Good catch.
Done!


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:345
+                                ArrayRef<OpFoldResult> values,
+                                std::function<bool(int64_t)> isDynamic) {
+  AffineExpr productOfValues = builder.getAffineConstantExpr(1);
----------------
ftynse wrote:
> qcolombet wrote:
> > 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.
> Nit: `function_ref` if the function isn't stored.
Good point!


================
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)(
----------------
chelini wrote:
> qcolombet wrote:
> > 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.
> maybe `ReassociativeReshapeLikeOp` ? Just got the name from: https://github.com/llvm/llvm-project/blob/ea32658288d449a01c89188a440a53aaed2563c8/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td#L1370 
Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134826