Page MenuHomePhabricator

[MLIR] Avoid adding extra memref::AllocOp in bufferize pass.

Authored by wenzhicui on Sep 7 2021, 6:38 PM.



Avoid adding extra memref::AllocOp in bufferize pass.

Diff Detail

Event Timeline

wenzhicui created this revision.Sep 7 2021, 6:38 PM
wenzhicui requested review of this revision.Sep 7 2021, 6:38 PM
mehdi_amini added inline comments.Sep 7 2021, 7:36 PM

Should there be something like CHECK-NOT: memref.alloc here? If I understand correctly you're avoiding generating a second alloc? (assuming this test case had two generated?)
Otherwise the test does not really demonstrate what I understand from the description.

wenzhicui updated this revision to Diff 371256.Sep 7 2021, 10:43 PM

Added CHECK-NOT as suggested by the reviewer.

wenzhicui added inline comments.Sep 7 2021, 10:45 PM

Thanks for the suggestion. Added the CHECK. That was the intention of this patch.

mehdi_amini added inline comments.Sep 7 2021, 11:00 PM

I don't think it is satisfying actually: the test seems to be passing without the code change. Can you look into this?

mehdi_amini added inline comments.Sep 7 2021, 11:01 PM

I see this IR in the test output before the code change:

18:  func @init_tensor(%arg0: tensor<?xf32>, %arg1: index) -> tensor<?xf32> { 
19:  %0 = memref.buffer_cast %arg0 : memref<?xf32> 
20:  %1 = memref.alloc(%arg1) : memref<?xf32> 
21:  linalg.generic {indexing_maps = [#map, #map], iterator_types = ["parallel"]} ins(%0 : memref<?xf32>) outs(%1 : memref<?xf32>) {
wenzhicui added inline comments.Sep 8 2021, 1:44 AM

I looked into this test and I realized that the CSE/Canonicalize pass is removing the extra memref.alloc ops because those ops are not used by any other ops. Do you think this patch is still needed since we can always have a CSE/Canonicalize pass to remove the extra alloc ops?

This should be reviewed by @herhut @pifon2a and DFKI friends

herhut requested changes to this revision.Sep 8 2021, 7:31 AM

This change is not correct in general I believe.

The existing alloc is the one for the initial value (e.g. init_tensor). However, there is no guarantee that this buffer is only passed to one linalg.generic operation. It might actually be reused. Hence, we have to create a copy if the value is required (see couple lines below the change). Otherwise, we allocate a fresh result buffer and only use the shape of the provided buffer.

If it then later turns out that the passed buffer indeed is not used anywhere else, we clean this up.

This revision now requires changes to proceed.Sep 8 2021, 7:31 AM
wenzhicui abandoned this revision.Sep 8 2021, 5:02 PM

Stephan/Mehdi, thanks for the review. Will abandon this change since it is incorrect.