Skip to content

[SLP] Increase UsesLimit to 64 #99467

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
Jul 19, 2024
Merged

Conversation

cyyself
Copy link
Contributor

@cyyself cyyself commented Jul 18, 2024

Since commit 82b800e addressed the issue #99327 , we see some performance regression (13%) on some verilator generated C++ code. This is because the UsesLimit is set to 8, which is too small for the verilator generated code. I have analyzed the need for the UsesLimit from [1] and found that the UsesLimit should be at least 64 to cover most of these cases. Thus, This patch increases the UsesLimit to 64.

Link: #99327 (comment) [1]

Since commit 82b800e addressed the
issue llvm#99327 , we see some performance regression (13%) on some
verilator generated C++ code. This is because the UsesLimit is set to
8, which is too small for the verilator generated code. I have analyzed
the need for the UsesLimit from [1] and found that the UsesLimit should
be at least 64 to cover most of these cases. Thus, This patch increases
the UsesLimit to 64.

Link: llvm#99327 (comment) [1]

Signed-off-by: Yangyu Chen <[email protected]>
@llvmbot
Copy link
Member

llvmbot commented Jul 18, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yangyu Chen (cyyself)

Changes

Since commit 82b800e addressed the issue #99327 , we see some performance regression (13%) on some verilator generated C++ code. This is because the UsesLimit is set to 8, which is too small for the verilator generated code. I have analyzed the need for the UsesLimit from [1] and found that the UsesLimit should be at least 64 to cover most of these cases. Thus, This patch increases the UsesLimit to 64.

Link: #99327 (comment) [1]


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+1-1)
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index d88c6307b994b..6f0878d5384be 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -209,7 +209,7 @@ static const unsigned AliasedCheckLimit = 10;
 
 // Limit of the number of uses for potentially transformed instructions/values,
 // used in checks to avoid compile-time explode.
-static constexpr int UsesLimit = 8;
+static constexpr int UsesLimit = 64;
 
 // Another limit for the alias checks: The maximum distance between load/store
 // instructions where alias checks are done.

@dtcxzyw dtcxzyw requested review from RKSimon and alexey-bataev July 18, 2024 10:29
@RKSimon
Copy link
Collaborator

RKSimon commented Jul 18, 2024

I've created a branch (perf/slp_usecase_limit) to determine the effect on compile time - it should appear on https://llvm-compile-time-tracker.com/?config=Overview&stat=instructions%3Au&remote=RKSimon in the next hour

@dtcxzyw
Copy link
Member

dtcxzyw commented Jul 18, 2024

@alexey-bataev
Copy link
Member

@dtcxzyw
Copy link
Member

dtcxzyw commented Jul 18, 2024

https://llvm-compile-time-tracker.com/compare.php?from=da0c8b275564f814a53a5c19497669ae2d99538d&to=23a9e6da423b4c6a1fda68d3d497a32ff2b04ec0&stat=instructions:u

Looks good in general (?)

Yeah. Surprisingly it also improves the performance of clang stage2-build.

@alexey-bataev
Copy link
Member

https://llvm-compile-time-tracker.com/compare.php?from=da0c8b275564f814a53a5c19497669ae2d99538d&to=23a9e6da423b4c6a1fda68d3d497a32ff2b04ec0&stat=instructions:u

Looks good in general (?)

Yeah. Surprisingly it also improves the performance of clang stage2-build.

Most probably, it has lots of uses, which must be replaced by the new extractelement instructions. And if the number is >8 but <64, better to walk over actual uses, rather than all uses (including those, potentially deleted/dropped a bit later)

Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

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

LG

@dtcxzyw dtcxzyw merged commit 007aa6d into llvm:main Jul 19, 2024
10 checks passed
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Since commit 82b800e addressed the
issue #99327 , we see some performance regression (13%) on some
verilator generated C++ code. This is because the UsesLimit is set to 8,
which is too small for the verilator generated code. I have analyzed the
need for the UsesLimit from [1] and found that the UsesLimit should be
at least 64 to cover most of these cases. Thus, This patch increases the
UsesLimit to 64.

Link:
#99327 (comment)
[1]

Signed-off-by: Yangyu Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants