[PATCH] D78484: Providing buffer assignment for MLIR

[Ehsan via Phabricator via llvm-commits]

dfki-ehna marked 2 inline comments as done.
dfki-ehna added inline comments.


================
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:
> dfki-mako wrote:
> > 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.
> If that future work could remove the use of MemRefType to instead use a TypeConverter that'd be awesome. Is there a bug you are using to track this work that I could follow along on to see when it lands? I really like the behavior of the pass and am excited to plug it in to our stuff :)
@benvanik We are definitely going to use TypeConverter instead either in this CL or in the following up one. Are you referring to an open discussion issue?


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:422
+      auto arg = block.getArgument(i);
+      arg.setType(toMemrefConverter(arg.getType()));
+    }
----------------
rriddle wrote:
> This isn't really valid to do directly in a pattern, as it is being done outside of the rewriter. Seems like this pattern can just be replaced by using a TypeConverter instead.
TypeConverter has convertBlockSignature which returns SignatureConversion but there is no applySignatureConversion for the rewriter that gets a Block as an input (the current version only accepts a region). Are we missing the point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78484