Skip to content

Commit 126b10e

Browse files
committed
Address review comments.
This partially incorporates llvm#89552. I haven't exposed it on canonicalizer pass as that could be distinct discussion.
1 parent dc43c03 commit 126b10e

File tree

2 files changed

+8
-14
lines changed

2 files changed

+8
-14
lines changed

mlir/include/mlir/Transforms/GreedyPatternRewriteDriver.h

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,13 @@ class GreedyRewriteConfig {
9292
/// An optional listener that should be notified about IR modifications.
9393
RewriterBase::Listener *listener = nullptr;
9494

95-
// Whether this should fold while greedily rewriting.
96-
//
97-
// Note: greedy here generally refers to two forms, 1) greedily applying
98-
// patterns based purely on benefit and applying without backtracking using
99-
// default cost model, 2) greedily folding where possible while attempting to
100-
// match and rewrite using the provided patterns. With this option set to
101-
// false it only does the former.
95+
/// Whether this should fold while greedily rewriting. This also disables
96+
/// CSE'ing constants.
10297
bool fold = true;
98+
99+
/// If set to "true", constants are CSE'd (even across multiple regions that
100+
/// are in a parent-ancestor relationship).
101+
bool cseConstants = true;
103102
};
104103

105104
//===----------------------------------------------------------------------===//

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -840,11 +840,6 @@ LogicalResult RegionPatternRewriteDriver::simplify(bool *changed) && {
840840
// regions to enable more aggressive CSE'ing).
841841
OperationFolder folder(ctx, this);
842842
auto insertKnownConstant = [&](Operation *op) {
843-
// This hoisting is to enable more folding, so skip checking if known
844-
// constant, updating dense map etc if not doing folding.
845-
if (!config.fold)
846-
return false;
847-
848843
// Check for existing constants when populating the worklist. This avoids
849844
// accidentally reversing the constant order during processing.
850845
Attribute constValue;
@@ -857,13 +852,13 @@ LogicalResult RegionPatternRewriteDriver::simplify(bool *changed) && {
857852
if (!config.useTopDownTraversal) {
858853
// Add operations to the worklist in postorder.
859854
region.walk([&](Operation *op) {
860-
if (!insertKnownConstant(op))
855+
if (!config.cseConstants || !insertKnownConstant(op))
861856
addToWorklist(op);
862857
});
863858
} else {
864859
// Add all nested operations to the worklist in preorder.
865860
region.walk<WalkOrder::PreOrder>([&](Operation *op) {
866-
if (!insertKnownConstant(op)) {
861+
if (!config.cseConstants || !insertKnownConstant(op)) {
867862
addToWorklist(op);
868863
return WalkResult::advance();
869864
}

0 commit comments

Comments
 (0)