Skip to content

Commit 16b78d0

Browse files
[mlir][Transforms] GreedyPatternRewriteDriver: Hash ops separately
The greedy pattern rewrite driver has multiple "expensive checks" to detect invalid rewrite pattern API usage. As part of these checks, it computes fingerprints for every op that is in scope, and compares the fingerprints before and after an attempted pattern application. Until now, each computed fingerprint took into account all nested operations. That is quite expensive because it walks the entire IR subtree. It is also redudant in the expensive checks because we already compute a fingerprint for every op. This commit significantly improves the running time of the "expensive checks" in the greedy pattern rewrite driver. Depends on #78306. Review only the top commit.
1 parent 6e6aa44 commit 16b78d0

File tree

3 files changed

+29
-21
lines changed

3 files changed

+29
-21
lines changed

mlir/include/mlir/IR/OperationSupport.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1316,10 +1316,10 @@ LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
13161316
//===----------------------------------------------------------------------===//
13171317

13181318
/// A unique fingerprint for a specific operation, and all of it's internal
1319-
/// operations.
1319+
/// operations (if `includeNested` is set).
13201320
class OperationFingerPrint {
13211321
public:
1322-
OperationFingerPrint(Operation *topOp);
1322+
OperationFingerPrint(Operation *topOp, bool includeNested = true);
13231323
OperationFingerPrint(const OperationFingerPrint &) = default;
13241324
OperationFingerPrint &operator=(const OperationFingerPrint &) = default;
13251325

mlir/lib/IR/OperationSupport.cpp

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -911,11 +911,12 @@ static void addDataToHash(llvm::SHA1 &hasher, const T &data) {
911911
ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(&data), sizeof(T)));
912912
}
913913

914-
OperationFingerPrint::OperationFingerPrint(Operation *topOp) {
914+
OperationFingerPrint::OperationFingerPrint(Operation *topOp,
915+
bool includeNested) {
915916
llvm::SHA1 hasher;
916917

917-
// Hash each of the operations based upon their mutable bits:
918-
topOp->walk([&](Operation *op) {
918+
// Helper function that hashes an operation based on its mutable bits:
919+
auto addOperationToHash = [&](Operation *op) {
919920
// - Operation pointer
920921
addDataToHash(hasher, op);
921922
// - Parent operation pointer (to take into account the nesting structure)
@@ -944,6 +945,12 @@ OperationFingerPrint::OperationFingerPrint(Operation *topOp) {
944945
// - Result types
945946
for (Type t : op->getResultTypes())
946947
addDataToHash(hasher, t);
947-
});
948+
};
949+
950+
if (includeNested)
951+
topOp->walk(addOperationToHash);
952+
else
953+
addOperationToHash(topOp);
954+
948955
hash = hasher.result();
949956
}

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,9 @@ struct ExpensiveChecks : public RewriterBase::ForwardingListener {
6060
void computeFingerPrints(Operation *topLevel) {
6161
this->topLevel = topLevel;
6262
this->topLevelFingerPrint.emplace(topLevel);
63-
topLevel->walk([&](Operation *op) { fingerprints.try_emplace(op, op); });
63+
topLevel->walk([&](Operation *op) {
64+
fingerprints.try_emplace(op, op, /*includeNested=*/false);
65+
});
6466
}
6567

6668
/// Clear all finger prints.
@@ -95,7 +97,8 @@ struct ExpensiveChecks : public RewriterBase::ForwardingListener {
9597
// API.) Finger print computation does may not crash if a new op was
9698
// created at the same memory location. (But then the finger print should
9799
// have changed.)
98-
if (it.second != OperationFingerPrint(it.first)) {
100+
if (it.second !=
101+
OperationFingerPrint(it.first, /*includeNested=*/false)) {
99102
// Note: Run "mlir-opt -debug" to see which pattern is broken.
100103
llvm::report_fatal_error("operation finger print changed");
101104
}
@@ -125,25 +128,23 @@ struct ExpensiveChecks : public RewriterBase::ForwardingListener {
125128

126129
protected:
127130
/// Invalidate the finger print of the given op, i.e., remove it from the map.
128-
void invalidateFingerPrint(Operation *op) {
129-
// Invalidate all finger prints until the top level.
130-
while (op && op != topLevel) {
131-
fingerprints.erase(op);
132-
op = op->getParentOp();
133-
}
131+
void invalidateFingerPrint(Operation *op) { fingerprints.erase(op); }
132+
133+
void notifyBlockRemoved(Block *block) override {
134+
RewriterBase::ForwardingListener::notifyBlockRemoved(block);
135+
136+
// The block structure (number of blocks, types of block arguments, etc.)
137+
// is part of the fingerprint of the parent op.
138+
// TODO: The parent op fingerprint should also be invalidated when modifying
139+
// the block arguments of a block, but we do not have a
140+
// `notifyBlockModified` callback yet.
141+
invalidateFingerPrint(block->getParentOp());
134142
}
135143

136144
void notifyOperationInserted(Operation *op,
137145
OpBuilder::InsertPoint previous) override {
138146
RewriterBase::ForwardingListener::notifyOperationInserted(op, previous);
139-
// Invalidate the finger print of the op that owns the block into which the
140-
// op was inserted into.
141147
invalidateFingerPrint(op->getParentOp());
142-
143-
// Also invalidate the finger print of the op that owns the block from which
144-
// the op was moved from. (Only applicable if the op was moved.)
145-
if (previous.isSet())
146-
invalidateFingerPrint(previous.getBlock()->getParentOp());
147148
}
148149

149150
void notifyOperationModified(Operation *op) override {

0 commit comments

Comments
 (0)