[PATCH] D133166: [mlir][MemRef] Canonicalize extract_strided_metadata(subview)

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi @nicolasvasilache ,

Let me know what you want to do with respect to the `Affine` dialect. (Whether we should use it or not here.)

I feel that if we want to go in that direction, we would need to do this canonicalization somewhere else (i.e. out of the core MemRef dialect).
I am happy to go down that road if you feel that's the way to go, just tell me where this would fit.

For now, I've stuck with the `arith` expansions, with all the problems there are dealing with it. (See inlined comments.)

Cheers,
-Quentin



================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1222
+      Value subStride;
+      if (subview.isDynamicStride(i))
+        subStride = subview.getDynamicStride(i);
----------------
qcolombet wrote:
> 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.
I ended up giving up on OpFoldResult here because unlike the AffineOps, the ArithmeticOps don't play nicely with that type.

Well more precisely, you need to convert them to actual values to be able to construct the arith operations, unless I missed something.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1271
+    // the subview.
+    for (int i = 0; i < sourceRank; ++i) {
+      if (droppedDims.test(i))
----------------
qcolombet wrote:
> 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!
I ended up populating the resulting strides and sizes in the same loop and I didn't see a good refactoring at this point.

I decided to go this way, because we already have to loop through the dimensions to populate the strides.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166