[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:1228
+
+      Value newStride = rewriter.createOrFold<arith::MulIOp>(
+          origLoc, newExtractStridedMetadata.getStrides()[i], subStride);
----------------
qcolombet wrote:
> 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).
Alright reporting back on this.

If we do this, the `MemRef` dialect will depend on the `Affine` dialect, which I am not sure is what the community wants in general.

On top of that, the `Affine` dialect already depends on the `MemRef` dialect, so we would create a circular dependency. That may not be a deal breaker, but I don't have enough MLIR experience at this point to know how it could be broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166