[PATCH] D135837: [mlir][MemRef] Simplify extract_strided_metadata(reinterpret_cast)

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked an inline comment as done.
qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:670
+/// base, baseOffset, ... = extract_strided_metadata(src)
+/// offset = baseOffset + srcOffset
+/// sizes = srcSizes
----------------
qcolombet wrote:
> nicolasvasilache wrote:
> > This feels very fishy .. I would expect just srcOffset here.
> > 
> > Computing the sum is a "subview"-like semantics, reinterpret_cast should just specify the offset UIAM
> That makes sense.
> I thought it was adding say offset to the given src_memref, but indeed that's not really "reinterpret-y".
> 
> Fixing.
Fixed.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:690
+    // Check if the source is suitable for extract_strided_metadata.
+    if (!reinterpretCastOp.getSource().getType().isa<MemRefType>())
+      return rewriter.notifyMatchFailure(
----------------
nicolasvasilache wrote:
> qcolombet wrote:
> > Is there a generic way of doing that?
> > Something like `ExtractStridedMetadata::verifySourceType`?
> Implement your own ?
I'll look into that as a follow up patch if that works for you, since if this doesn't exist yet, it will likely require some tablegen work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135837