[PATCH] D78484: Providing buffer assignment for MLIR

[Marcel Koester via Phabricator via llvm-commits]

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