[PATCH] D135736: [mlir][MemRef] Make reinterpret_cast(extract_strided_metadata) more robust

[Quentin Colombet via Phabricator via llvm-commits]

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

Hi @nicolasvasilache,

Please take another look.

I've introduced new methods to get the most constant values of the sizes, strides, and offset for `extract_strided_metadata` and `reinterpret_cast` (see `getConstifiedMixedXXX` methods; BTW suggestions for a better name are appreciated!)

The "complexity" is now all hidden in `constifyIndexValues` and the `reinterpret_cast(extract_strided_metadata)` combine looks straight forward (no zip, no function ptr indirection etc.).

Cheers,
-Quentin



================
Comment at: mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td:930
+    /// Similar to `getConstifiedMixedSizes` but for the offset.
+    OpFoldResult getConstifiedMixedOffset();
+  }];
----------------
We'll probably want these methods to be part of an interface.
`ViewLikeInterface` could have been a candidate but `extract_strided_metadata` doesn't conform to it, so we likely will need to introduce something else.
For now, I just added the methods where I needed them.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:145
+    llvm::function_ref<SmallVector<int64_t>(MemRefType)> getAttributes,
+    llvm::function_ref<bool(int64_t)> isDynamic) {
+  SmallVector<int64_t> constValues = getAttributes(memRefTy);
----------------
I acknowledge that this API is complicated, but it is not meant to be used directly.
This represents the meat of what the `getConstifiedMixedXXX` methods do, thus we have to support `size`, `strides`, and `offset`.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:169
+      ofr = builder.getIndexAttr(
+          ofr.get<Attribute>().cast<IntegerAttr>().getInt());
+      continue;
----------------
I'll do an RFC on how to properly fix that.
I have a wip patch for that, but it's getting longer than expected to fix all the places.

I wanted to unblock this patch, hence the workaround.
Let me know if you want me to file a bug for the proper fix.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1432
+  atLeastOneReplacement |= replaceConstantUsesOf(
+      builder, getLoc(), getStrides(), getConstifiedMixedStrides());
 
----------------
@nicolasvasilache 
This refactoring is not required but allows to remove some code.
I'm happy to leave it out if you prefer the previous version.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1843
+    hasKnownStridesAndOffset = succeeded(
+        getStridesAndOffset(extractStridedMetadata.getSource().getType(),
+                            extractStrides, extractOffset));
----------------
nicolasvasilache wrote:
> can we add this to the op itself to return a vector of opfoldresult which best effort contains the Value or Attr extract from the type.
Done in `getConstifiedMixedXXX` methods.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135736