Skip to content

[MLIR] Don't check for key before inserting in map in GreedyPatternRewriteDriver worklist (NFC) #88148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

joker-eph
Copy link
Collaborator

@joker-eph joker-eph commented Apr 9, 2024

This is a common anti-pattern (any volunteer for a clang-tidy check?).

This does not show real word significant impact though.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Apr 9, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 9, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Mehdi Amini (joker-eph)

Changes

This is a common anti-pattern (any volunteer for a clang-tidy check?).

Amazingly a micro-benchmark of the GreedyPatternRewriteDriver shows a 2.5x speedup of the entire GreedyPatternRewriteDriver execution with this patch.


Full diff: https://github.com/llvm/llvm-project/pull/88148.diff

1 Files Affected:

  • (modified) mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp (+1-2)
diff --git a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
index bbecbdb8566935..604d8628fae755 100644
--- a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
+++ b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
@@ -243,9 +243,8 @@ bool Worklist::empty() const {
 void Worklist::push(Operation *op) {
   assert(op && "cannot push nullptr to worklist");
   // Check to see if the worklist already contains this op.
-  if (map.count(op))
+  if (map.insert({op, list.size()}).second)
     return;
-  map[op] = list.size();
   list.push_back(op);
 }
 

Copy link
Contributor

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! A clang tidy check would indeed be good. I've written the correct form thousands of times and (almost) always go out of my way to do it, but I often have to hit the docs to remember exactly which form of insert/second does what I want. It's not surprising to me that people can't remember this and just write the simpler to understand form thinking it won't hurt too much.

…writeDriver worklist (NFC)

This is a common anti-pattern (any volunteer for a clang-tidy check?).

Amazingly a micro-benchmark of the GreedyPatternRewriteDriver shows
a 2.5x speedup of the entire GreedyPatternRewriteDriver execution
with this patch.
@joker-eph joker-eph force-pushed the double-hash-lookup branch from 2054ff4 to 4018f03 Compare April 9, 2024 17:12
@joker-eph
Copy link
Collaborator Author

Just taking back the benchmark claim, my microbenchmark was busted, I dug a bit more and can't get very significant real-world impact.

Still a straightforward minor cleanup though.

@stellaraccident
Copy link
Contributor

stellaraccident commented Apr 9, 2024

Oh well, still a good cleanup. I was trying to figure out how in this case, it could be such an extreme benefit. My conception of reality likes that it is not a huge difference in this case, but the speedup would have been nice :)

@joker-eph joker-eph merged commit 60c5c4c into llvm:main Apr 9, 2024
@joker-eph joker-eph deleted the double-hash-lookup branch April 9, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants