[PATCH] D78484: Providing buffer assignment for MLIR

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.
Herald added a subscriber: Kayjukh.


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:14
+
+#ifndef MLIR_TRANSFORM_BUFFERPLACEMENT_H
+#define MLIR_TRANSFORM_BUFFERPLACEMENT_H
----------------
typo TRANSFORM -> TRANSFORMS


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:21
+#include "mlir/IR/Operation.h"
+#include "mlir/Support/LLVM.h"
+#include "mlir/Transforms/DialectConversion.h"
----------------
This header isn't necessary I believe.


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:69
+  explicit BufferAssignmentOpConversionPattern(
+      MLIRContext *context_,
+      BufferAssignmentPlacer *bufferAssignment_ = nullptr,
----------------
Please remove the trailing _ from these variable names.


================
Comment at: mlir/include/mlir/Transforms/Passes.td:109
+    This pass implements an algorithm to optimize the placement of alloc and
+    dealloc operations. This pass also inserts missing dealloc operations
+    automatically to reclaim memory.
----------------
Can you add some example input/output here?


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:95
+      return;
+    for (auto alias : it->second)
+      resolveRecursive(alias, result);
----------------
nit: auto -> Value


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:183
+      return BufferPlacementPositions(result.getOwner(), result.getOwner());
+    // Get all possible aliases
+    auto possibleValues = aliases.resolve(result);
----------------
nit: Add punctuation here.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:350
+      auto deallocs = analysis.findAssociatedDeallocs(alloc);
+      assert(deallocs.size() < 2 &&
+             "Not supported number of associated dealloc operations");
----------------
Can you emit an error and use `signalPassFailure()` instead of assert here?


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:422
+      auto arg = block.getArgument(i);
+      arg.setType(toMemrefConverter(arg.getType()));
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78484