[PATCH] D77487: [MLIR] Introduce applyOpPatternsAndFold for op local rewrites

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

I have a growing concern that rebuilding the pattern list is going to become extremely costly for compile time. I don't have a concrete suggestion at the moment, but we should avoid rebuilding if possible.

That being said, I think this is a great start to doing local canonicalization. Thank you for pushing this!



================
Comment at: mlir/include/mlir/IR/PatternMatch.h:464
+/// apply any patterns recursively to the regions of `op`.
+bool applyOpPatternsAndFold(Operation *op,
+                            const OwningRewritePatternList &patterns,
----------------
I'm of the opinion that we should remove the return value from these methods. It is not used anywhere, and it isn't clear what the user would use it for in any of the existing usages.


================
Comment at: mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp:236
+    if (isa<AffineLoadOp>(op)) {
+      AffineLoadOp::getCanonicalizationPatterns(patterns, &getContext());
+    } else {
----------------
This seems extremely costly. Can we create these once and just apply them if necessary?


================
Comment at: mlir/lib/Dialect/Affine/Transforms/SimplifyAffineStructures.cpp:95
+    if (isa<AffineForOp>(op))
+      AffineForOp::getCanonicalizationPatterns(patterns, op->getContext());
+    else if (isa<AffineIfOp>(op))
----------------
Same here. Can we avoid rebuilding a pattern set for each operation?


================
Comment at: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:275
+
+  // These are hooks implemented for PatternRewriter.
+protected:
----------------
Please use /// here an below.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:318
+      if (res) {
+        /// Simplify/canonicalize the affine.for.
+        OwningRewritePatternList patterns;
----------------
nit: Use // here.


================
Comment at: mlir/test/lib/Dialect/Affine/TestAffineDataCopy.cpp:100
+        promoteIfSingleIteration(forOp);
+      else if (auto loadOp = dyn_cast<AffineLoadOp>(op))
+        copyOps.push_back(loadOp);
----------------
nit: `isa<>() || isa<>()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77487