[PATCH] D78498: [mlir] [linalg] Only promote selected buffers.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/LinalgTransforms.h:124
 
+SmallVector<Value, 0> promoteSelectedSubviewsLinalgOpAndSetMarker(
+    PatternRewriter &rewriter, Operation *op,
----------------
Please document this function


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgTransforms.cpp:342
+  LinalgOp linOp = cast<LinalgOp>(op);
+  SmallVector<int64_t, 4> toPromote;
+  for (int64_t i = 0; i < linOp.getNumInputsAndOutputBuffers(); ++i)
----------------
Please `reserve` the space before populating a vector in a loop


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgTransforms.cpp:343
+  SmallVector<int64_t, 4> toPromote;
+  for (int64_t i = 0; i < linOp.getNumInputsAndOutputBuffers(); ++i)
+    toPromote.push_back(i);
----------------
`int64_t i = 0, e = linOp.getNumInputsAndOutputBuffers(); i < e; ++i)` is a common LLVM pattern that avoids calling the function in a loop


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgTransforms.cpp:374
+    auto newOp = promoteSubViewOperands(rewriter, linOp, subViews);
+    if (linalgMarker != "")
+      newOp.setAttr(LinalgTransforms::kLinalgTransformMarker,
----------------
Nit: `!linalgMarker.empty()` may be more efficient


================
Comment at: mlir/test/lib/DeclarativeTransforms/TestLinalgTransformPatterns.td:153
+def : Pat<(MatmulOp:$op $_, $_, $_),
+          (PromoteSelectedSubviewsLinalgOp<[0, 1], "first_view_promotion">),
+          [(Constraint<And<[
----------------
I don't follow this. The test name suggests you promote the _first_ view, but the list contains operands with indices 0 and 1... The .mlir file also seems to check only for one copy. Consider using only `[0]` here and `CHECK-NOT` for extra copies before matmul.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78498