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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:88
+    bool hasKnownStridesAndOffset =
+        succeeded(getStridesAndOffset(sourceType, sourceStrides, sourceOffset));
+    // Helper function to get either the value of the newly created
----------------
Is it possible for the `getStridesAndOffset` method to fail on the source of a `subview`?

Put differently, how can I write a test for that part?


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/SimplifyExtractStridedMetadata.cpp:107
+      return origStrides[idx];
+    };
+
----------------
FWIW, I'm not super happy with this helper function, because we end up materializing (and then deleting later) constants.

I've tried to use the `getStridesAndOffset` function directly with the `AffineExpr` constructs, but I was not satisfied with the code.

Essentially, I would end up in the main loop below with something like:
```
AffineConstantExpr cst;

if (known  && (cst = strideExpr.dyn_cast<AffineConstantExpr>())) {
  makeComposedFolded(s0 * cst, {subStride})
} else {
  makeComposedFolded(s0 * s1, {subStride, origStride})
}
```

The main problem here is that the number of operands is different (`subStride` vs. `subStride, origStride`), so getting rid of the if is not easy (at least I didn't see a nice way to do it.)

What I tried for that was something like:
```
Expr = s0
Operands = {subStride}
If ()
  Expr *= cst
Else {
  Expr *= s1
 Operands.push_back(origStride)
}
 makeComposedFolded(s0 * Expr, Operands)
```

And I didn't find it particularly readable.


================
Comment at: mlir/test/Dialect/MemRef/simplify-extract-strided-metadata.mlir:71
+  %base_buffer, %offset, %sizes:3, %strides:3 = memref.extract_strided_metadata %subview : memref<?x6x3xf32, strided<[64,4,1], offset: 210>> -> memref<f32>, index, index, index, index, index, index, index
+  return %base_buffer, %offset, %sizes#0, %sizes#1, %sizes#2, %strides#0, %strides#1, %strides#2 : memref<f32>, index, index, index, index, index, index, index
+}
----------------
@nicolasvasilache I've kept some of the mixed dynamic/static tests.
Compared to the full dynamic test (`extract_strided_metadata_of_subview_all_dynamic` at the end of this file), these tests don't add much, but I feel they are more approachable than dissecting the huge affine map of the last test.

Let me know if you want to remove them nonetheless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133166