[PATCH] D136377: [mlir][MemRefToLLVM] Reuse existing subview lowering

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Thanks for the history here @ftynse !

To give a little bit of context, Nicolas realized that some code that currently lives in `memref-to-llvm` has to be reimplemented when doing `memref-to-something-else` (e.g., like memref to vmvx in the IREE compiler https://github.com/iree-org/iree/blob/main/compiler/src/iree/compiler/Dialect/VMVX/Transforms/ResolveBufferDescriptors.cpp#L21.). (In particular all the code about figuring out the strides, sizes, and offset for view like operations.) The goal was to move the common part of this lowering in core MLIR, i.e., what we do in `populateSimplifyExtractStridedMetadataOpPatterns`, but that common part cannot obviously target the final dialect (LLVM or something else). We settle on affine for the intermediate step (but using arith would yield the exact same problems), hence we need an additional lowering step.

> Furthermore, having the memref-to-llvm _pass_ subsume patterns that the populateMemRefToLLVM does not contain will break such uber-lowering passes and require surprising, in my opinion, modifications to fix them.

I didn't get that part. `populateMemRefToLLVMConversionPatterns` is augmented to include all the required patterns in that patch. Thus uber-lowering passes would work just fine unless I missed something.
Anyhow, that doesn't address the core issue that we don't want to pull `affine-to-std` and/or `arith-to-llvm`.

I don't think that's something possible in the current framework, but to be NFC(-ish) here we would need to only apply the `affine-to-std` and `arith-to-llvm` to the view like operations, not all operations.

> Conceptually, the right solution for me would be to separate the part that produces non-LLVM operations into a separate "expand-memref-ops" pass like we have for math, and then just lower the resulting mix with the existing passes.

That sounds exactly what we did with the `SimplifyExtractStridedMetadata` pass (which we should probably rename in `expand-memref-ops` given how it evolved).

It looks like I am missing only one thing: how do you connect the pipeline so that it works for all the users of `memref-to-llvm`? We cannot expect all of them to call `expand-memref-ops` before calling `memref-to-llvm`, can we?

I didn't see how it works for the math dialect: I see the expand pass, but I don't see how it is connected with the conversion math-to-llvm conversion.

Cheers,
-Quentin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136377