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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Thanks for the quick review @chelini !



================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:231
+
+    // The result value will start with the base_buffer and offset.
+    unsigned baseIdxInResult = 2;
----------------
chelini wrote:
> I would mention explicitly that this pattern does not affect base buffer and offset, as mentioned in the test cases.
Good point!
It is not that clear in the high level comment of that pattern/commit message too since I changed the name of the `offset` value (from `offset` to `baseOffset`).
I'll fix that and also call it out.


================
Comment at: mlir/test/Dialect/MemRef/simplify-extract-strided-metadata.mlir:317
+//          = 1 * 2
+//          = 1
+// stride 4 = baseStrides#1
----------------
chelini wrote:
> 
Good catch x)


================
Comment at: mlir/test/Dialect/MemRef/simplify-extract-strided-metadata.mlir:399
+// stride 4 = baseStrides#1 * size 5 * size 6
+//          = baseStrides#1 * (baseSizes#1 / 30) * 3
+// stride 5 = baseStrides#1 * size 6
----------------
BTW something I haven't done yet is making sure this gets simplified into `baseSizes#1 / 10` since at this point we know that `baseSizes#1` is divisible by both `10` and `3`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133625