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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
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";
+  }
----------------
nicolasvasilache wrote:
> ftynse wrote:
> > 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.
> strong +1 to this.
> 
> Cutting a little deeper to drop the driver like I am doing here https://reviews.llvm.org/D143943 would be very welcome.
IOW this is a simple additional refactoring:
  1. extract the functional-style transformation into its own well-defined API,
  2. have the pattern merely wrap calls to that API + populate method,
  3. transform dialect op calls the API directly.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144108