[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:330
+/// the right positions. It uses the algorithm described at the top of the file.
+// TODO(dfki): create a templated version that allows to match dialect-specific
+// alloc/dealloc nodes and to insert dialect-specific dealloc node.
----------------
rriddle wrote:
> rriddle wrote:
> > nit: Please remove the `(username)`
> Generally using traits and interfaces are how you should generalize a pass, instead of templating.
Definitely. We are going to make this pass more generic in one of the next CLs.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:388
+  return opBuilder.saveInsertionPoint();
+}
+
----------------
mehdi_amini wrote:
> This does not seem to be "computing" anything? Is this like "future work"?
This function does not "compute" anything. This is just a simple abstraction to make the integration into the dialect-specific legalization phases more convenient and pratical with respect to future extensions. For instance, adding support for additional types and interfaces might require an extension of this functionality.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:398
+    ConversionPatternRewriter &rewriter) const {
+  auto toMemrefConverter = [&](Type t) -> Type {
+    if (auto tensorType = t.dyn_cast<RankedTensorType>())
----------------
benvanik wrote:
> How tied is this pass to memref? If we have our own dialect type that represents buffers that we want to use with our own dialect alloc/dealloc ops, how can we use that here?
> 
> Specifically this kind of function type conversion seems better served by a TypeConverter that can be provided by the target dialect. For us, for example, we'd not have it change types at all probably, and instead just use this for inserting our alloc/dealloc markers.
Currently, the implementation is strongly coupled to memref types. However, this only affects the helper converters provided. The underlying pass will be extended in a follow-up CL to work on alloc and free interfaces instead of AllocOp and DeallocOp. This will make the general pass compatible with arbitrary dialects that implement the required interfaces.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:464
+      "Executes buffer assignment pass to automatically move alloc and dealloc "
+      "operations into their proper positions");
+}
----------------
mehdi_amini wrote:
> It seems to me that the pass is a misnomer: it does not really "assign" buffers, but optimizes the placement, would there be a more accurate name?
> 
> Also saying "into their proper positions" seems like it is intended for correctness.
I guess a better name would be "BufferPlacement", since the pass moves allocations/deallocations into "better" positions.


================
Comment at: mlir/test/lib/Transforms/TestBufferAssignment.cpp:57
+          return rewriter.notifyMatchFailure(
+              op, "dynamic shapes not currently supported");
+        auto memrefType =
----------------
silvas wrote:
> How would this pass be extended to support dynamic shapes? It would be good to have that written down in a comment (and hopefully implemented as a fast follow-on to this CL).
We can add more specific comments about the feature you mentioned. Adding support for dynamic shapes would be in fact on the next follow-up CLs. One thing that has to be adapted is the computation of the alloc and free positions. Moreover, it might be necessary to adapt the BufferAssignmentPlacer (however, I don't think so at the moment).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78484/new/

https://reviews.llvm.org/D78484