[PATCH] D79927: [MLIR] [Linalg] Add option to use the partial view after promotion.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:101
+  // If true the full view should be used for the promoted buffer. If false, use
+  // the partial view.
+  Optional<SmallVector<bool, 4>> useFullTileBuffers = None;
----------------
And if None?


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:102
+  // the partial view.
+  Optional<SmallVector<bool, 4>> useFullTileBuffers = None;
+  LinalgPromotionOptions &setUseFullTileBuffers(ArrayRef<int64_t> uses) {
----------------
You can also consider `llvm::BitVector`


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:104
+  LinalgPromotionOptions &setUseFullTileBuffers(ArrayRef<int64_t> uses) {
+    useFullTileBuffers = SmallVector<bool, 4>(uses.begin(), uses.end());
+    return *this;
----------------
The API looks misleading, `ArrayRef<int64_t> uses` feels like it accepts the indices of operands for which full tiles must be used, but you really imply `ArrayRef<bool> useFullTiles)`.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:109
+  LinalgPromotionOptions &setUseFullTileBuffers(bool use) {
+    useFullTileBuffers = SmallVector<bool, 4>(1, use);
+    return *this;
----------------
This looks like it would use full buffer for the first operand, not all operands.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:73
+  unsigned nBuffers = linalgOp.getNumInputsAndOutputBuffers();
+  SmallVector<bool, 4> vUseFullTileBuffers(nBuffers, false);
+  if (options.useFullTileBuffers.hasValue()) {
----------------
```
SmallVector<bool, 4> vUseFullTileBuffers = options.useFullTileBuffers.getValueOr(SmallVector<bool, 4>());
vUseFullTileBuffers.resize(linalgOp.getNumInputsAndOutputBuffers(), false);
```

looks more concise to me


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:85
         subViews.insert(sv);
+        this->useFullTileBuffers[sv] = vUseFullTileBuffers[idx];
+      }
----------------
Nit: I think `this->` is unncessary here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79927