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

Thu Oct 27 07:42:08 PDT 2022

nicolasvasilache added a comment.

In D136377#3887824 <https://reviews.llvm.org/D136377#3887824>, @ftynse wrote:

>> 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.
>
> Thanks. The implementation of "extract metadata" was a bit rushed and this explanation wasn't sufficiently clear before.

How was this rushed? It was deliberately done at a snails' pace over 3+weeks between the beginning of the discussion and the op implementation landing.
The explanation is pretty clearly discussed here: https://discourse.llvm.org/t/extracting-dynamic-offsets-strides-from-memref/64170/9 LLVM is but one target of interest.

> This approach to factoring out the common code may be overusing the pattern idea, surely there are other ways of sharing the implementation.
> IMO, we could have just introduced a helper function along the lines of "emit arith dialect IR equivalent to applying the given affine map to the given operands" and called it from several patterns instead of emitting an "affine.apply" (with all the baggage it brings) and then applying another pattern to lower it.

I don't see value in jumping that abstraction gap and introducing a significantly more convoluted implementation for a small subset of functionality.
This is akin the reimplementing canonicalizations inside of a pass rather than using the proper things and should actively be discouraged.

Additionally, this helper function along the lines of "emit arith dialect IR equivalent .." still does not solve the arith-to-llvm part.

> I am really not a fan of the layering here. This goes back to what we had with uber-passes "convert-X-Y-Z-to-A-B-C" with big dependencies. It is very surprising that running convert-memref-to-llvm will also convert any affine operations directly to LLVM, instead of lowering them to Arith where they can be jointly optimized with other arithmetic operations.

No question that we should avoid pulling the whole of affine-to-llvm and arith-to-llvm transitively with memref-to-llvm.
My thinking was to just bring in the AffineApplyLowering pattern but 1. even that may be too intrusive and 2. it still does not solve the arith-to-llvm problem.
I don't think selectively adding rewrite patterns to conversions is a problem.
Are you suggesting this as a general problem?
If so, it seems you are proposing that conversion infra should never be able to call rewrite patterns (didn't look deep into the implications here but my gut feeling is it seems too restrictive).

Stepping back, this is the classic ordering problems we get with dialects + progressive lowering: dialects percolate ops together into clusters and dependences arise.
The traditional solution is to split ops into new dialects; I am not clear it would make sense to put memref.extract_metadata into another dialect (but maybe it does?)

An orthogonal concern is that all these memref-to-llvm and other conversion passes remain **test** passes that we insist on string-chaining together via mlir-opt and handwaving that into a "compiler".
As long as we don't have a ConvertToLLVM and insist on peer-to-peer conversions by proxy, these issues will continue popping over and over again.
A notional ConvertToLLVM could detect which dialects are registered and adds all the relevant lowerings in the proper order.
Not having such a batteries-included pass makes MLIR unnecessarily hard to use, having folks run around digging for unspoken phase orderings.

For the time being, to unblock progress, I would:

1. avoid populating the world in this pass and I would update the test to run `mlir-opt --expand-memref-ops --affine-to-std --arith-to-llvm --memref-to-llvm`
2. prefix all the "-to-llvm" passes with "test" to make it clear what their intent is
3. start a convert-to-llvm if we care about making MLIR usable.

@ftynse any objection to 1. ? (or a better proposal?)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136377