https://github.com/erichkeane updated https://github.com/llvm/llvm-project/pull/211368
>From 770775a58491e1b3a387ce772ed0437c53017867 Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Tue, 21 Jul 2026 16:15:18 -0700 Subject: [PATCH 1/3] [CIR] Switch FlattenCFGPass to no longer use greedy manager. This showed up doing a self-build of MLIR's Presburger IntegerRelation.cpp, which is a bit pathalogical. It resulted in us doing a lot of rewrite patterns during flatten, taking about 20s. After this patch, we're down to sub-1s spent doing that. This is because applyOpPatternsGreedily was re-enqueing every child opops every time we modified anything nearby. This caused us in cases where there were operations that were visited TONS of times just because a parent got modified. This patch replaces this with a very simple inside-out iteration of these operations. The recent loop-op 'cleanup' flattening modification necessitates us re-visiting these sometimes (hence the loop). This patch is effectively 'NFC' other than build time, so there really isn't a test I could write. AI: Note: I've used Claude Opus 4.8 to help me with this patch. I've read/attempted to comprehend as much of this as possible, but I'm still pretty inexperienced as to how to manage passes/etc. Claude promises me this is the best way, and I haven't been able to find anything better grepping around other transformations in other projects, but please comment if you have a better idea! --- .../lib/CIR/Dialect/Transforms/FlattenCFG.cpp | 57 +++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp index 160b6418528fc..64bb5c4e5282f 100644 --- a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp +++ b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp @@ -17,9 +17,9 @@ #include "mlir/IR/Builders.h" #include "mlir/IR/PatternMatch.h" #include "mlir/Interfaces/SideEffectInterfaces.h" +#include "mlir/Rewrite/PatternApplicator.h" #include "mlir/Support/LogicalResult.h" #include "mlir/Transforms/DialectConversion.h" -#include "mlir/Transforms/GreedyPatternRewriteDriver.h" #include "clang/CIR/Dialect/IR/CIRDataLayout.h" #include "clang/CIR/Dialect/IR/CIRDialect.h" #include "clang/CIR/Dialect/Passes.h" @@ -682,9 +682,9 @@ class CIRLoopOpInterfaceFlattening // exceptions that might be thrown from the step region. Rather than trying // to figure out all of the cleanup routing here, we sink the condition into // the body region, hoist the step region (if any) and create a new - // cir.cleanup.scope enclosing the body region. Subsequent passes of the - // greedy driver will flatten the cir.cleanup.scope and the loop reusing - // the normal handlers. + // cir.cleanup.scope enclosing the body region. A subsequent sweep of the + // pass will flatten the cir.cleanup.scope and the loop reusing the normal + // handlers. if (op.maybeGetCleanup()) return rewriteLoopWithCleanup(op, rewriter); @@ -2001,20 +2001,43 @@ void populateFlattenCFGPatterns(RewritePatternSet &patterns) { } void CIRFlattenCFGPass::runOnOperation() { - RewritePatternSet patterns(&getContext()); - populateFlattenCFGPatterns(patterns); - - // Collect operations to apply patterns. - llvm::SmallVector<Operation *, 16> ops; - getOperation()->walk<mlir::WalkOrder::PostOrder>([&](Operation *op) { - if (isa<IfOp, ScopeOp, SwitchOp, LoopOpInterface, TernaryOp, CleanupScopeOp, - TryOp>(op)) - ops.push_back(op); - }); + RewritePatternSet patternList(&getContext()); + populateFlattenCFGPatterns(patternList); + FrozenRewritePatternSet patterns(std::move(patternList)); + + PatternApplicator applicator(patterns); + // We need _A_ cost model, and everything here is the same cost-model, so this + // is effectively a no-op, but necessary to use the PatternApplicator. + applicator.applyDefaultCostModel(); + + mlir::PatternRewriter rewriter(&getContext()); + + + bool changed; + do { + changed = false; + // Collect flatten candidates post-order so an inner op is handled before + // its parent; op pointers stay valid across the block splits / region + // inlines the patterns perform (a pattern only erases the matched op and + // its descendants, which are visited first), so the list can be iterated + // directly. + llvm::SmallVector<Operation *, 16> ops; + getOperation()->walk<mlir::WalkOrder::PostOrder>([&](Operation *op) { + if (isa<IfOp, ScopeOp, SwitchOp, LoopOpInterface, TernaryOp, + CleanupScopeOp, TryOp>(op)) + ops.push_back(op); + }); - // Apply patterns. - if (applyOpPatternsGreedily(ops, std::move(patterns)).failed()) - signalPassFailure(); + for (mlir::Operation *op : ops) { + rewriter.setInsertionPoint(op); + if (mlir::succeeded(applicator.matchAndRewrite(op, rewriter))) { + // A vast majority of these don't modify the structured ops. However, + // if they do, we have to try again. Store whether we've made any + // modifications and try again until we stop changing anything. + changed = true; + } + } + } while (changed); } } // namespace >From dc63e4a54afcbad0fc6d36b3f313d29609d3f409 Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Wed, 22 Jul 2026 15:13:03 -0700 Subject: [PATCH 2/3] Remove extraneous newline, per clang format --- clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp index 64bb5c4e5282f..fee319c93eb22 100644 --- a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp +++ b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp @@ -2012,7 +2012,6 @@ void CIRFlattenCFGPass::runOnOperation() { mlir::PatternRewriter rewriter(&getContext()); - bool changed; do { changed = false; >From 59888d7735f1284cab7ef8c8c09f51577112a264 Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Thu, 23 Jul 2026 06:47:32 -0700 Subject: [PATCH 3/3] Add mutation-listening type to keep an eye on whether stuff has changed. --- .../lib/CIR/Dialect/Transforms/FlattenCFG.cpp | 50 +++++++++++++++---- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp index fee319c93eb22..eda0b59cc2367 100644 --- a/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp +++ b/clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp @@ -2000,6 +2000,41 @@ void populateFlattenCFGPatterns(RewritePatternSet &patterns) { patterns.getContext()); } +namespace { +// An implementation of RewriterBase::Listener that determines whether the IR +// has been modified since the last time it was 'reset'. At the moment, this is +// the only use for something like this, but we might wish to move this +// somewhere if someone else needs similar functionality in the future. +class MLIRChangedListener final : public mlir::RewriterBase::Listener { + bool hasChanged = false; + +public: + void reset() { hasChanged = false; } + + bool changed() const { return hasChanged; } + + void notifyBlockErased(Block *) override { hasChanged = true; } + void notifyOperationModified(Operation *) override { hasChanged = true; } + void notifyOperationReplaced(Operation *, Operation *) override { + hasChanged = true; + } + void notifyOperationReplaced(Operation *, ValueRange) override { + hasChanged = true; + } + void notifyOperationErased(Operation *) override { hasChanged = true; } + + // notifyPatternBegin, notifyPatternEnd, notifyMatchFailure all skipped, since + // they don't modify. + void notifyOperationInserted(Operation *, + mlir::IRRewriter::InsertPoint) override { + hasChanged = true; + } + void notifyBlockInserted(Block *, Region *, Region::iterator) override { + hasChanged = true; + } +}; +} // namespace + void CIRFlattenCFGPass::runOnOperation() { RewritePatternSet patternList(&getContext()); populateFlattenCFGPatterns(patternList); @@ -2011,10 +2046,12 @@ void CIRFlattenCFGPass::runOnOperation() { applicator.applyDefaultCostModel(); mlir::PatternRewriter rewriter(&getContext()); + MLIRChangedListener changedListener; + rewriter.setListener(&changedListener); - bool changed; do { - changed = false; + changedListener.reset(); + // Collect flatten candidates post-order so an inner op is handled before // its parent; op pointers stay valid across the block splits / region // inlines the patterns perform (a pattern only erases the matched op and @@ -2029,14 +2066,9 @@ void CIRFlattenCFGPass::runOnOperation() { for (mlir::Operation *op : ops) { rewriter.setInsertionPoint(op); - if (mlir::succeeded(applicator.matchAndRewrite(op, rewriter))) { - // A vast majority of these don't modify the structured ops. However, - // if they do, we have to try again. Store whether we've made any - // modifications and try again until we stop changing anything. - changed = true; - } + (void)applicator.matchAndRewrite(op, rewriter); } - } while (changed); + } while (changedListener.changed()); } } // namespace _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
