Page MenuHomePhabricator

[mlir] Decompose Bufferization Clone operation into Memref Alloc and Copy.
ClosedPublic

Authored by dfki-jugr on Fri, Nov 19, 4:18 AM.

Details

Summary

This patch introduces a new conversion to convert bufferization.clone operations
into a memref.alloc and a memref.copy operation. This transformation is needed to
transform all remaining clones which "survive" all previous transformations, before
a given program is lowered further (to LLVM e.g.). Otherwise, these operations
cannot be handled anymore and lead to compile errors.
See: https://llvm.discourse.group/t/bufferization-error-related-to-memref-clone/4665

Diff Detail

Event Timeline

dfki-jugr created this revision.Fri, Nov 19, 4:18 AM
dfki-jugr requested review of this revision.Fri, Nov 19, 4:18 AM
dpotop accepted this revision.Fri, Nov 19, 9:40 AM

This is the first time I am solicited as a reviewer, probably because I raised the issue on discourse (https://llvm.discourse.group/t/bufferization-error-related-to-memref-clone/4665).
The patch implements a solution to the problem we signalled.
The code seems complete, and a test has been provided.
I am not yet competent enough to judge conformance to coding rules. If you think this should have made me resign as a reviewer, please let me know.

This revision is now accepted and ready to land.Fri, Nov 19, 9:40 AM
herhut added inline comments.Mon, Nov 22, 1:33 AM
mlir/lib/Conversion/MemRefToMemRef/MemRefToMemRef.cpp
23 ↗(On Diff #388458)

This only works for static shapes.

mlir/test/Conversion/MemRefToMemRef/memref-to-memref.mlir
4 ↗(On Diff #388458)

Please add a test with dynamic shapes and unknown rank.

I think it is ok if this fails for unknown rank for now but at least dynamic shapes should be supported.

herhut requested changes to this revision.Mon, Nov 22, 1:34 AM
This revision now requires changes to proceed.Mon, Nov 22, 1:34 AM
pifon2a requested changes to this revision.Mon, Nov 22, 1:36 AM
pifon2a added inline comments.
mlir/include/mlir/Conversion/Passes.td
355

Why is it not a "transform"? I thought that when we stay within a dialect, then it should be a transformation, not a conversion.

dfki-jugr added inline comments.Mon, Nov 22, 4:25 AM
mlir/include/mlir/Conversion/Passes.td
355

The idea was, that we could simply rename it to "bufferize-to-memref", if a bufferize dialect will be created. In this case, it would be a conversion. But you are right, if we stay within a dialect it should be a transformation.

I suggest to move it to transforms, because this is the right place for this and both renaming and moving is almost the same amount of work.

pifon2a added inline comments.Mon, Nov 22, 4:31 AM
mlir/include/mlir/Conversion/Passes.td
355

Since the bufferization dialect is almost landed, let's just wait a bit and not submit. Then you will be able to just rename it. In the meantime, you can address Stephan's comments.

bondhugula added inline comments.
mlir/test/Conversion/MemRefToMemRef/memref-to-memref.mlir
10–13 ↗(On Diff #388458)

No numbered values in CHECK lines.

dfki-jugr updated this revision to Diff 389208.Tue, Nov 23, 7:51 AM

Add dynamic shaped allocs and test cases.
Addressed comments.

dfki-jugr marked 3 inline comments as done.Tue, Nov 23, 7:53 AM
dfki-jugr added inline comments.
mlir/include/mlir/Conversion/Passes.td
355

Sounds good. Just tell me when it is landed.

bondhugula added inline comments.Sat, Nov 27, 3:45 AM
mlir/include/mlir/Conversion/Passes.td
356

The description looks too generic -- perhaps rephrase to " ... to simpler ones in the MemRef ...".

358

I think you are missing the dependent dialects specification.

mlir/lib/Conversion/MemRefToMemRef/MemRefToMemRef.cpp
6 ↗(On Diff #389208)

File-level summary comment here.

20 ↗(On Diff #389208)

Summary doc comment.

mehdi_amini added inline comments.Sun, Nov 28, 11:10 AM
mlir/include/mlir/Conversion/Passes.td
355
dfki-jugr marked 3 inline comments as done.

All files are renamed to "bufferization to memref" after the bufferization
dialect has been created.
Addressed further comments.

dfki-jugr edited the summary of this revision. (Show Details)Mon, Nov 29, 1:00 AM
dfki-jugr retitled this revision from Decompose MemRef Clone operation into Alloc and Copy. to [mlir] Decompose Bufferization Clone operation into Memref Alloc and Copy..
herhut requested changes to this revision.Mon, Nov 29, 1:12 AM
herhut added inline comments.
mlir/include/mlir/Conversion/Passes.td
134

Does this need to be a module pass? This limits concurrency and I think the transformation done here is function local.

mlir/lib/Conversion/BufferizationToMemRef/BufferizationToMemRef.cpp
27

nit: transforms.

38

This limits composition. As an alternative, have the pattern fail and return a failure reason via the rewriter.

48

nit: would memrefType.isDynamicDim(i) work here, too?

50

Should this be a DimOp here?

63

Are these comments needed?

This revision now requires changes to proceed.Mon, Nov 29, 1:12 AM
dfki-jugr updated this revision to Diff 390294.Mon, Nov 29, 2:33 AM
dfki-jugr marked 6 inline comments as done.
dfki-jugr edited the summary of this revision. (Show Details)

Addressed comments.

pifon2a accepted this revision.Mon, Nov 29, 2:38 AM
pifon2a added inline comments.
mlir/include/mlir/Conversion/Passes.td
137

nit: sort dialects

mlir/lib/Conversion/BufferizationToMemRef/BufferizationToMemRef.cpp
15–22

sort headers?

herhut accepted this revision.Mon, Nov 29, 6:50 AM

Please sort the headers, otherwise looks good. Thanks!

This revision is now accepted and ready to land.Mon, Nov 29, 6:50 AM
mehdi_amini added inline comments.Mon, Nov 29, 4:37 PM
mlir/include/mlir/Conversion/Passes.td
135

Can you expand on the doc a bit, at least mention that it turns a clone into alloc+copy.
I'd also point to the end-to-end doc and mention that this pass alone won't resolve leaks.

This revision was automatically updated to reflect the committed changes.
dfki-jugr marked an inline comment as done.