[PATCH] D78484: Providing buffer assignment for MLIR
Marcel Koester via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 22 04:49:26 PDT 2020
dfki-mako added inline comments.
================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:194
+ /// information.
+ DeallocSetT findAssociatedDeallocs(AllocOp alloc) const {
+ DeallocSetT result;
----------------
mehdi_amini wrote:
> Isn't there a trait we could use instead of hard-coding the `AllocOp` here (and everywhere)?
There is a `MemoryEffectsOpInterface` that could be used in favor of hard code standard allocation operations. However, I would prefer making BA more generic in a follow-up CL. Furthermore, this would require [[ https://reviews.llvm.org/D78619 | this CL ]] to be merged first in order to support standard Alloc and Dealloc nodes.
================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:368
+ }
+ });
+ };
----------------
mehdi_amini wrote:
> Isn't this all assuming a particular model with a single allocation / deallocation per buffer?
> What about a pattern like the following with two allocs feeding into one dealloc? (we can imagine more complex cases)
>
> ```
> cond_br %cond, ^bb1, ^bb2
> ^bb1:
> %buffer1 = alloc ...
> br ^exit(%buffer1 : ....)
> ^bb2:
> %buffer2 = alloc ...
> br ^exit(%buffer2 : ...)
> ^exit(%buffer: ...):
> dealloc %buffer
> ```
This pass currently assumes a single-allocation/deallocation model that usually appears during straight-forward legalization (lowering) of operations. We wanted to keep the first version simple and would like to significantly extend the functionality in one of the follow-up CLs.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78484/new/
https://reviews.llvm.org/D78484
More information about the llvm-commits
mailing list