[PATCH] D79518: [mlir][Linalg] Introduce a helper function for staged pattern application

[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:375
+/// global transformations, in a staged fashion:
+///   1. the first stage consists of an OwningRewritePatternList. The
+///   RewritePattern in this list are applied once and in order.
----------------
it's a list of OwningRewritePatternLists


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:376
+///   1. the first stage consists of an OwningRewritePatternList. The
+///   RewritePattern in this list are applied once and in order.
+///   2. the second stage consists of a single OwningRewritePattern that is
----------------
this does not match the code


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:377
+///   RewritePattern in this list are applied once and in order.
+///   2. the second stage consists of a single OwningRewritePattern that is
+///   applied greedily until convergence.
----------------
OwningRewritePatternList


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:384
+void applyStagedPatterns(
+    Operation *op, SmallVector<OwningRewritePatternList, 4> &stage1Patterns,
+    OwningRewritePatternList &stage2Patterns,
----------------
ArrayRef


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:385
+    Operation *op, SmallVector<OwningRewritePatternList, 4> &stage1Patterns,
+    OwningRewritePatternList &stage2Patterns,
+    std::function<void(Operation *)> stage3Lambda = nullptr);
----------------
`const &`


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp:206
+    std::function<void(Operation *)> stage3Lambda) {
+  for (auto &patterns : stage1Patterns) {
+    if (!applyPatternsAndFoldGreedily(op, patterns))
----------------
const &


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp:208
+    if (!applyPatternsAndFoldGreedily(op, patterns))
+      llvm::dbgs() << "Underlying pattern rewrite did not converge";
+    if (!applyPatternsAndFoldGreedily(op, stage2Patterns))
----------------
May be you want to report which stage


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp:208
+    if (!applyPatternsAndFoldGreedily(op, patterns))
+      llvm::dbgs() << "Underlying pattern rewrite did not converge";
+    if (!applyPatternsAndFoldGreedily(op, stage2Patterns))
----------------
ftynse wrote:
> May be you want to report which stage
Don't we want to bail out if we could not converge in one of the applications?


================
Comment at: mlir/test/lib/Transforms/TestLinalgTransforms.cpp:176
+
+  patternsVector.emplace_back(std::move(
+      OwningRewritePatternList().insert<LinalgPromotionPattern<MatmulOp>>(
----------------
Could we just add a constructor template to OwningRewritePatternList that forwards its arguments to insert?  This would remove the need to `std::move` and call `insert` here, and looks generally useful for lists that need only one call to `insert`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79518