-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Commit 21f7cf4057b7 causing huge regression in compile time #48033
Comments
Sorry, provided the wrong git commit hash for the commit that introduced the issue. The correct commit is 21f7cf4. |
Are you seeing the perf problem in a debug build? If the perf concern is with a release+asserts build, then we can wrap the verify call inside a debug macro (similar to what it was before https://reviews.llvm.org/D80401 )? diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
Can you try it with your test file and see if that solves the slowdown? |
We are using a Release+Asserts build for our testing. I tried out the change you suggested and it gets compile times back to normal for the test in question. Unless there is a reason not to, could you commit your proposed change? |
Committed with: |
I don't think LLVM_DEBUG does what you want it to. I think you should consider EXPENSIVE_CHECKS instead. LLVM_DEBUG is defined here, conditional on NDEBUG, which is what controls assertions, so LLVM_DEBUG will run in Release+Asserts builds: If I'm reading it correctly, LLVM_DEBUG only runs when the See also the bug I filed about verifyFunction being too slow: |
Thanks for the explanation and bug link! The post-commit comments suggested my patch wasn't the right fix as well: I'll post a new proposal on Phab, so we can reach a better conclusion. |
|
I went ahead and committed the expensive checks guard within SLP here because I think that's better than what I had before: bug 47712, comment 1 suggests a potential improvement: add an expensive checks guard within Verifier::verifyFunctionMetadata() or just outside that. If we have that, we can probably remove the check in SLP (effectively revert that pass to where we started). Douglas - can you verify that perf is still ok with your test and resolve this bug if so? |
Thanks for the fix Sanjay, the compile performance of the test is back to a tiny amount (150 seconds before vs. 0.245 seconds after). |
Extended Description
We have a large automatically generated test internally which a while back started taking massively longer to compile. From the compiler time report, the cause of the increase was the SLP Vectorizer pass which went from <0.5 seconds to around 180 seconds which I bisected to commit e6251ad4caef.
The large test contains 1000 automatically generated test cases, and I did several runs and calculated what % of the time was spent in the SLP Vectorizer pass. When limiting the test to only 50 cases, the compiler only spent 3.6% of the time in the SLP Vectorizer. At 200 test cases, it was up to 25%, 450 was 55%, up to 950 cases where we were spending roughly 71% of the compilation time in the SLP Vectorizer.
It was suggested that perhaps we can be smarter about when we run the SLP Vectorizer pass in order to avoid this huge compile time regression. Possibly by only making the call to verifyFunction run at the end of or after the BoUpSLP destructor, so we only need to make it so that it only runs when a change has been made. This could be achieved by passing "changed" into BoUpSLP, or by calling it just after runImpl is called (in SLPVectorizer::runOnFunction and SLPVectorizerPass::run).
The text was updated successfully, but these errors were encountered: