[PATCH] D144108: [mlir][LinAlg][Transform] Add a transform op for conv2d to im2col

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

These patterns look very similar, any chance they can be generalized somehow?



================
Comment at: mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td:1682
+    Applies im2col to every supported operation in the given target.
+    This is a wrapper around populateConvertConv2DToImg2ColPatterns.
+
----------------
It would be nice to give a more ample description. This is user-visible on the website...


================
Comment at: mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td:1685
+    Return modes:
+    =============
+    Returns a definite failure if target is not isolated from above.
----------------
Please do not use `===` with is the h1 header. Use leading `####` instead to get a h4 that this should be.


================
Comment at: mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td:1690-1691
+
+  let arguments = (ins PDL_Operation:$target);
+  let results = (outs PDL_Operation:$transformed);
+
----------------
New operations must no longer hardcode `PDL_Operation` as a type. Use the type `TransformHandleTypeInterface`instead and specify the type in the assembly format.


================
Comment at: mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp:3049-3052
+  if (failed(applyPatternsAndFoldGreedily(target, std::move(patterns)))) {
+    return emitSilenceableFailure(target->getLoc())
+           << "failed to apply img2col";
+  }
----------------
Do we actually need the entire greedy rewriter for this, including all the pseudo-canonicalization and DCE stuff it carries? This is applying three patterns that seem mutually exclusive. Can't we just apply patterns one-by-one? With a tiny bit of effort, we can even be more informative as to what didn't match.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:31
+                       OpBuilder &builder) {
+  if (isInt)
+    return builder.create<arith::AddIOp>(loc, x, y);
----------------
Can't we query this from `x.getType()`?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:90
+    if (!filterType.hasStaticShape() || !inputType.hasStaticShape()) {
+      return failure();
+    }
----------------
Nit: return `rewriter.notifyMatchFailure` helps with debugging.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:101
+
+    auto filterShape = filterType.getShape();
+    auto outputShape = outputType.getShape();
----------------
Nit: please expand auto.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:104
+
+    const int n = outputShape[0];
+    const int oh = outputShape[1];
----------------
We don't do `const` variables in MLIR.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:114
+
+    SmallVector<int64_t, 4> colTensorShape = {n, oh, ow, fh, fw, ic};
+
----------------
Nit: drop the number of stack elements, here and below.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:373
+
+    auto transposedResult =
+        transposeOperand(batchMatVecResultReshaped, {0, 2, 3, 1});
----------------
Nit: please expand `auto` when the type is reasonable to spell and not obvious from the context. I've got no idea what it should be here.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/ConvertConv2DToImg2Col.cpp:385
+// (i.e. (D, C x Kh x Kw) * (C x Kh x Kw, Ho x Wo))
+class ConvertConv2DNchwFchw final
+    : public OpRewritePattern<linalg::Conv2DNchwFchwOp> {
----------------
Same comments as above apply below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144108