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

Fri Oct 28 04:30:34 PDT 2022

ftynse added a comment.

In D136377#3888537 <https://reviews.llvm.org/D136377#3888537>, @nicolasvasilache wrote:

> 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.

The referenced post does not seem to discuss the lowering, only the operation motivated by downstream needs. Furthermore, it states that "Our objective is to get rid of that descriptor ourselves by folding it away before LLVM", which seems to be the exact opposite of what is being implemented: extracting elements from the descriptor while keeping it alive and hoping that LLVM will simplify it. What is being implemented is, however, consistent with the state of the lowering //before// that proposal, which I described as "the common wisdom was that something will do SRoA and DCE at a lower level (LLVM dialect or LLVM IR) anyway" in https://discourse.llvm.org/t/extracting-dynamic-offsets-strides-from-memref/64170/16. I did not have concerns with materializing the descriptor as a data structure or with doing address arithmetic at the arith level, potentially even at the affine level. I do have concerns with this happening inside the LLVM-specific lowering pass (contrary to the motivation of the LLVM being only one of targets) and exercised only indirectly.

> 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.

This still brings in a dependency on Affine, which is rather heavy and mostly unnecessary... We have repeatedly mentioned the idea of having a variant of affine.apply that wasn't part of the affine dialect, but it never materialized.

> Separately, 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).

I specifically object to having affine-to-std conversion patterns subsumed by memref-to-llvm conversion pattern set because "memref-to-llvm" does not mention neither affine nor any dialect that used to be std, and to this tendency to subsume one broad set of patterns in another in general. This has nothing to do with some patterns being implemented as ConversionPattern or RewritePattern. We have ~50 conversion passes and as many pattern groups only in the main repo. This is already difficult to comprehend and manage.

While this was not a part of my reasoning above, I do think that conversion infra should not call rewrite patterns. Patterns are written taking into account the inner workings of a specific driver, whether we want it or not. The differences between the greedy and the conversion driver are numerous and subtle. Here are some that come to mind: rewrite patterns can check for single-use or no-use of a value, conversion patterns cannot because the original uses //may or may not// be removed immediately (depending on the ops being replaced or updated in-place); rewrite patterns can be applied recursively provided there is a fixed point, conversion patterns cannot because they would produce an operation declared illegal (if it is declared legal, the pattern would never apply); the usual "thou shalt not mutate IR bypassing the rewriter" to which the conversion rewriter is sensitive //differently// than the greedy one though both will crash with abstruse memory corruption errors.

> 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

This seems to be what I suggested above, certainly I cannot object to what I said I thought was the right approach.

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

> 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.

> 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-opt usable.

There doesn't seem to be a consensus on these, and it is outside of the scope of this commit.

In D136377#3889166 <https://reviews.llvm.org/D136377#3889166>, @qcolombet wrote:

> @ftynse, for communicating this new requirement, should I just send a PSA on MLIR discourse, or you have something else in mind?

A PSA should do.

> I'll keep a separate test for the full convert MemRef to LLVM "pipeline".

You can define an actual named pass pipeline https://mlir.llvm.org/docs/PassManagement/#pass-pipeline-registration, this may also help users that want a one-step solution to lowering all of memref to llvm at any cost.

Also, IIRC there was a transform in the memref dialect (or maybe in affine) that would "normalize" memrefs with non-strided layout maps by shifting the layout into `affine.load` and/or `affine.apply` operations on operands and by making the memref have the default layout instead. If it is still around, we may consider a `memref-to-affine` transformation that combines that with what is being proposed here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136377