[PATCH] D78484: Providing buffer assignment for MLIR

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:21
+#include "mlir/IR/Operation.h"
+#include "mlir/Support/LLVM.h"
+#include "mlir/Transforms/DialectConversion.h"
----------------
dfki-ehna wrote:
> rriddle wrote:
> > This header isn't necessary I believe.
> BufferAssignmentOpConversionPattern inherits from OpConversionPattern in this file.
Sorry, I meant the one above it. Support/LLVM.h is included transitively.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:422
+      auto arg = block.getArgument(i);
+      arg.setType(toMemrefConverter(arg.getType()));
+    }
----------------
dfki-ehna wrote:
> 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?
When passing a type converter the non-entry blocks are converted automatically using that converter. After that I would expect that the default function conversion pattern would remove the need for this pattern:
https://github.com/llvm/llvm-project/blob/3d178581ac7f5336b1ac75e31001de074ecca937/mlir/include/mlir/Transforms/DialectConversion.h#L300


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78484