This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dfki-jugr on Nov 19 2021, 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.Nov 19 2021, 4:18 AM
dfki-jugr requested review of this revision.Nov 19 2021, 4:18 AM
dpotop accepted this revision.Nov 19 2021, 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.Nov 19 2021, 9:40 AM
herhut added inline comments.Nov 22 2021, 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.Nov 22 2021, 1:34 AM
This revision now requires changes to proceed.Nov 22 2021, 1:34 AM
pifon2a requested changes to this revision.Nov 22 2021, 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.Nov 22 2021, 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.Nov 22 2021, 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.Nov 23 2021, 7:51 AM

Add dynamic shaped allocs and test cases.
Addressed comments.

dfki-jugr marked 3 inline comments as done.Nov 23 2021, 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.Nov 27 2021, 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.Nov 28 2021, 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)Nov 29 2021, 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.Nov 29 2021, 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.Nov 29 2021, 1:12 AM
dfki-jugr updated this revision to Diff 390294.Nov 29 2021, 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.Nov 29 2021, 2:38 AM
pifon2a added inline comments.
mlir/include/mlir/Conversion/Passes.td
137

nit: sort dialects

mlir/lib/Conversion/BufferizationToMemRef/BufferizationToMemRef.cpp
14–21

sort headers?

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

Please sort the headers, otherwise looks good. Thanks!

This revision is now accepted and ready to land.Nov 29 2021, 6:50 AM
mehdi_amini added inline comments.Nov 29 2021, 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.
mehdi_amini added inline comments.Nov 30 2021, 11:06 PM
mlir/include/mlir/Conversion/Passes.td
135

Ping on this?

dfki-jugr added inline comments.Dec 1 2021, 4:39 AM
mlir/include/mlir/Conversion/Passes.td
135

I think, this is not the right place to mention that a clone operation is converted into alloc+copy, since this is a general description of the conversion pass. All other conversion passes also don't have a description or list of all converted operations. If there are further op-conversions added, the description has to be changed again. So, I added these remarks to the conversion implementation in the cpp file.
However, if this pass will not resolve memory leaks in general (also using other conversions), we can add a remark here.

mehdi_amini added inline comments.Dec 1 2021, 8:57 PM
mlir/include/mlir/Conversion/Passes.td
135

this is not the right place to mention that a clone operation is converted into alloc+copy

Well this is all of what this pass does right now, and the bufferization stuff is complex enough that I'll insist on better documentation and cross referencing.

this is a general description of the conversion pass

My point is that this summary right now is basically useless as a doc: this is purely redundant with the pass mnemonic without adding any new information.

The doc gets published on the website: https://mlir.llvm.org/docs/Passes/#-convert-bufferization-to-memref-convert-operations-from-the-bufferization-dialect-to-the-memref-dialect ; So please fix and improve.

You only added a summary (which isn't published for some reason), but there should be a description fields which can contains multiple paragraph as needed.
If you need inspiration, https://mlir.llvm.org/docs/Passes/#-lower-affine-lower-affine-operations-to-a-combination-of-standard-and-scf-operations

Please also consider any such comment as blocking by default in the future.

mehdi_amini added inline comments.Dec 7 2021, 2:42 AM
mlir/include/mlir/Conversion/Passes.td
135

Ping here?

dfki-jugr added inline comments.Dec 8 2021, 2:45 AM
mlir/include/mlir/Conversion/Passes.td
135

Added documentation here:
https://reviews.llvm.org/D115326

mehdi_amini added inline comments.Dec 8 2021, 9:30 AM
mlir/include/mlir/Conversion/Passes.td
135

Thanks!