[PATCH] D79271: [mlir][Linalg] Mostly NFC - Refactor Linalg patterns and transformations.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache created this revision.
Herald added subscribers: llvm-commits, Kayjukh, frgossen, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, antiagainst, shauheen, jpienaar, rriddle, mehdi_amini, mgorny.
Herald added a reviewer: aartbik.
Herald added a project: LLVM.
rriddle added a reviewer: rriddle.
rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:51
+
+//============================================================================//
+// Preconditions that ensure the corresponding transformation suceeds and can be
----------------
Please use the same header blocks as the rest of the infra.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:87
+  SmallVector<StringRef, 4> matchDisjunction;
+  llvm::Optional<StringRef> replacement;
+};
----------------
Drop the extraneous llvm::


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:92
+/// Linalg tiling patterns.
+///
+struct LinalgBaseTilingPattern : public RewritePattern {
----------------
These patterns seem light on documentation, can you add some?


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:127
+                               LinalgMarker marker = LinalgMarker(),
+                               int benefit = 1);
+  LogicalResult matchAndRewrite(Operation *op,
----------------
This should really be using PatternBenefit instead of int


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp:40
+
+//============================================================================//
+// Transformations exposed as rewrite patterns.
----------------
same here.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp:215
+    return failure();
+  if (!linalgOp)
+    return failure();
----------------
This looks duplicated


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:43
+    return false;
+  if (r.getBlocks().size() != 1)
+    return false;
----------------
Replace these two with llvm::hasSingleElement


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:46
+  auto &ops = r.front().getOperations();
+  if (ops.size() != 3)
+    return false;
----------------
llvm::hasNItems(r.front().begin(), r.front().end(), 3);


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:96
+  };
+  if (!llvm::all_of(genericOp.getInputsAndOutputBuffers(),
+                    isStaticMemRefWithIdentityLayout))
----------------
nit: return success(llvm::all_of(...));


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:139
+  return res.getDefiningOp();
+  ;
+}
----------------
Extraneous semicolon



Linalg transformations are currently exposed as DRRs.
Unfortunately RewriterGen does not play well with the line of work on named linalg ops which require variadic operands and results.
Additionally, DRR is arguably not the right abstraction to expose compositions of such patterns that don't rely on SSA use-def semantics.

This revision abandons DRRs and exposes manually written C++ patterns.

Refactorings and cleanups are performed to uniformize APIs.
This refactoring will allow replacing the currently manually specified Linalg named ops.

A collateral victim of this refactoring is the `tileAndFuse` DRR, and the one associated test, which will be revived at a later time.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79271

Files:
  mlir/include/mlir/Dialect/Linalg/CMakeLists.txt
  mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
  mlir/lib/Dialect/Linalg/Transforms/CMakeLists.txt
  mlir/lib/Dialect/Linalg/Transforms/Interchange.cpp
  mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
  mlir/lib/Dialect/Linalg/Transforms/LinalgTransforms.cpp
  mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
  mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
  mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
  mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
  mlir/test/Dialect/Linalg/transform-patterns.mlir
  mlir/test/lib/DeclarativeTransforms/CMakeLists.txt
  mlir/test/lib/DeclarativeTransforms/TestLinalgTransformPatterns.td
  mlir/test/lib/Transforms/CMakeLists.txt
  mlir/test/lib/Transforms/TestLinalgTransforms.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79271.261562.patch
Type: text/x-patch
Size: 78839 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200502/a0158d21/attachment-0001.bin>