[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/IR/MemRefOps.cpp:1222
+      Value subStride;
+      if (subview.isDynamicStride(i))
+        subStride = subview.getDynamicStride(i);
----------------
nicolasvasilache wrote:
> We should have something in StaticValueUtils.h|cpp to hide away the if/else complexity and return an OpFoldResult.
> Could you reuse or extend ?
Thanks for the pointer!
I dug a little bit more and I was struggling to find a generic way to describe the ValueRange we want to iterate on.

The high level interface is OffsetSizeAndStrideOpInterface and I thought of using that and then I stumbled on `OffsetSizeAndStrideOpInterface::getMixedStrides()` which covers that and returns directly `OpFoldResult`.

So looks like I may avoid the StaticValueUtils all together.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1228
+
+      Value newStride = rewriter.createOrFold<arith::MulIOp>(
+          origLoc, newExtractStridedMetadata.getStrides()[i], subStride);
----------------
nicolasvasilache wrote:
> Can we use a single AffineApplyOp here and below?
> At the higher levels of abstraction this is a better way of representing such indexing computations (i.e. 1 op instead of N ops).
> 
> See e.g. https://sourcegraph.com/github.com/llvm/llvm-project/-/blob/mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp?L154
> You can also make the variadic process of defining symbols bound to a position better by using something like: https://sourcegraph.com/github.com/llvm/llvm-project/-/blob/mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp?L281
Thanks for that pointer as well.
Makes a lot of sense.

However regarding the 1 op vs. N ops. That's going to be true for the offset, but not for the strides.
Indeed, we still need to produce one value per stride since these appear in the final result.

I need to look closer how `AffineApplyOp` are built to avoid creating useless one (when one of the stride is 1 or both strides are constant).


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1271
+    // the subview.
+    for (int i = 0; i < sourceRank; ++i) {
+      if (droppedDims.test(i))
----------------
nicolasvasilache wrote:
> Can this be factored out into a helper function that would live next to e.g. `SubViewOp::inferResultType` ?
> It could return a vector of OpFoldResult to avoid passing a builder and then StaticValueUtils can be used or extended to materialize the constants here.
> 
> All this should then become much more idiomatic (and shorter).
Thanks for the suggestion, looking!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166