Skip to content
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

[NFC] Improve DeduplicateExecutables bucketing #19601

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IanWood1
Copy link
Contributor

@IanWood1 IanWood1 commented Jan 3, 2025

This change uses the result types as a part of the hash when grouping ops. This vastly improves the performance of this pass when there are several similar objects that consist of ops with the same names but differ in the number/type of results. However, this may increase the overhead of hashing when bucketing isn't effective.

Although a sample size of one, I found that for 405b tp8, the number of buckets went from 35 -> 140. This brought the time of this pass from a few minutes to several seconds.

Use the result types as a part of the hash when grouping ops. This
vastly improves the performance of this pass when there are several
similar objects that have different types.

This pass can become very slow because checking structural equivalence is a
costly procedure pass is O(n^2) with respect to the bucket size.

Signed-off-by: Ian Wood <[email protected]>
@IanWood1 IanWood1 marked this pull request as ready for review January 4, 2025 23:55
Copy link
Contributor

@hanhanW hanhanW left a comment

Choose a reason for hiding this comment

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

LGTM, though I would not put NFC to the commit. It usually means that a patch is a pure refactoring/cleanup. However, the behavior is changed for some models..

https://llvm.org/docs/Lexicon.html#n

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants