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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

> We had this exact same discussion with @nicolasvasilache and thought it was fine.

I really don't think so. I have lost track of the time I had spent helping people debug surprising lowerings back in the day when we had the "standard" dialect that conflated everything. This was one of the driving factors to ultimately split the standard dialect into smaller pieces. We've collectively spend a significant amount of time to implement that change as well as to split the lowerings into isolated sets of patterns, exercised by the respective passes. As much as I dislike that process, I am going to have to ask you to post an RFC on the forum if you really want to revert that decision entirely or to say that A->B lowerings should be allowed to subsume lowerings from dialects other than A.

Now, I am aware that some downstream compilers, including the one that @nicolasvasilache is contributing to, have an uber-lowering pass that runs lowering patterns from a dozen dialects to LLVM in a single rewriter sweep. For that pass, it is okay to just depend on everything and emit intermediate ops from any dialect, but we haven't really designed upstream passes for this. 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.

Just to give an additional concrete example, "affine-to-standard" patterns include conversion from `affine.for` to `scf.for`, among other things. So it is not sufficient to just have "affine" as dependent dialect, so should be "scf" and a bunch of other things. Without that, the pass may trigger an assertion and stop the pipeline. Even with that, it will introduces ops from other dialects that need a two-stage lowering before they reach the LLVM dialect.

> The alternative I can see is we could declare affine.apply legal, so that they don't get expanded in the process. The problem with that is it is oddly specific.

This is slightly better. 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136377