[PATCH] D77487: [MLIR] Introduce applyOpPatternsAndFold for op local rewrites
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 14 23:57:37 PDT 2020
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
More information about the llvm-commits
mailing list