Page MenuHomePhabricator

[mlir][memref] Fix canonicalization of memref.clone
ClosedPublic

Authored by herhut on May 19 2021, 9:31 AM.

Details

Summary

The previous code was a bit too eager in canonicalizing.

Diff Detail

Event Timeline

herhut created this revision.May 19 2021, 9:31 AM
herhut requested review of this revision.May 19 2021, 9:31 AM

I think this should become a pass instead, so that it can also consider aliases and
remove more clones. Block local aliases would suffice to catch situations like

%931 = memref.reshape %930(%107) : (memref<*xf16>, memref<?xindex>) -> memref<*xf16>
%932 = memref.clone %931 : memref<*xf16> to memref<*xf16>
memref.dealloc %930 : memref<*xf16>
herhut updated this revision to Diff 346498.EditedMay 19 2021, 10:21 AM

Sent the wrong state of the change

Hey @dfki-jugr. I debugged this a bit today and here is how far I got. We can either land this as a cleanup or you can fold it into some changes you want to do anyway.

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
537

This is a dangerous folding, as it means that a clone can change a memref's type. One then has to take care of this in all patterns that want to drop a clone. I'd rather just keep the casting behavior separate.

mlir/lib/Transforms/BufferDeallocation.cpp
154

This is a unrelated minor bug.

dfki-jugr added inline comments.May 20 2021, 2:55 AM
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
508

Can we drop the last one in the block in this case?

527

nit: The last sentence is broken: information, of = if, drop on ...?

herhut updated this revision to Diff 347020.May 21 2021, 7:13 AM

Fix comments and re-enable cast handling

herhut marked an inline comment as done.
herhut added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
508

We should drop the earlier one, right? I won't add this now, maybe as a follow up.

bkramer accepted this revision.May 21 2021, 7:17 AM

lgtm

This revision is now accepted and ready to land.May 21 2021, 7:17 AM
This revision was landed with ongoing or failed builds.May 21 2021, 7:37 AM
This revision was automatically updated to reflect the committed changes.