[PATCH] D133625: [mlir][MemRef] Simplify extract_strided_metadata(expand_shape)

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:268
+
+      // Calculate product(expandShapeSize#j, for j in
+      // reassIdx+1..reassIdx+group.size-1) for each dimension of that group.
----------------
nicolasvasilache wrote:
> Can anything be refactored and reused from `MemRefOps.cpp::namespace saturated_arith {` ?
Thanks for the pointer, I didn't know about these utilities.

At first glance, it doesn't look like it would fit here.

In particular, here we don't saturate when we see a dynamic stride/size, we replace it by 1 and patch up the expression with the proper dynamic value later.

The suffix product that we do here is similar to what `computeExpandedLayoutMap` does, except in our case we don't want to saturate.

I'm guessing the suffix product is used in a lot of places given that's how strides are computed, maybe we can refactor on that front. Though how we deal with dynamic dimensions is where things diverge and I don't know how to reconcile that at the moment.

Put differently, I don't know yet what the refactoring should look like.

Do you want me to spend more time looking into this or should we move on for now and wait for more use cases to show up?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133625