[Mlir-commits] [mlir] [MLIR][Bufferization] Bail on automatic deallocation to enable reentrant behaviour (PR #72289)

[Renato Golin]

rengolin wrote:

> This PR tries to add support for existing deallocation ops without considering all necessary cases. If we want to add support, we need to do it in a way that doesn't break the existing specification of the pass (no double free, etc.).

Absolutely agree. My PR was more asking the question than proposing a solution (hence some comments were incomplete). The answer is clear: not enough. :)

> If you have pre-existing deallocs in your IR, there are two options:
> 
> 1. Remove all deallocs and let the buffer deallocation pass re-insert them. We have a pass that does that for `memref.realloc`: `--expand-realloc=emit-deallocs=false`. Deallocs may not be inserted at the same position. E.g., deallocs will always be inserted in the same block as the alloc.
> 2. Annotate `memref.alloc`+`memref.dealloc` with `bufferization.manual_deallocation`. (You could do that after running the buffer deallocation pass.) Such ops will be ignored by the buffer deallocation pass. Note, this would not work with the example shown above if only one alloc has that attribute because the dealloc may receive a buffer that is not under manual deallocation.

If these pre-existing deallocs are added by the user or another pass, then these could be reasonable steps. But since they're added by the ownership pass itself, it wouldn't be self-consistent.

> Maybe we'd want a separate attribute, though, to differentiate manual from already automatically deallocated.

Perhaps. I assumed (from the field comment) that if the dealloc wasn't marked as `kManualDeallocation`, it was considered "automatically managed" without ownership. However, introducing another attribute will probably cascade checks through the entire pass.

Reading it again after your input, I think my understanding of the pass itself was mistaken. I'll have to re-evaluate the strategy, thanks for the feedback!


https://github.com/llvm/llvm-project/pull/72289