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

Commit 21f7cf4057b7 causing huge regression in compile time #48033

Closed
dyung opened this issue Jan 7, 2021 · 9 comments
Closed

Commit 21f7cf4057b7 causing huge regression in compile time #48033

dyung opened this issue Jan 7, 2021 · 9 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@dyung
Copy link
Collaborator

dyung commented Jan 7, 2021

Bugzilla Link 48689
Resolution FIXED
Resolved on Jan 14, 2021 03:26
Version trunk
OS All
CC @RKSimon,@rnk,@rotateright
Fixed by commit(s) 267ff79, 0aa75fb, 3f09c77

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).

@dyung
Copy link
Collaborator Author

dyung commented Jan 7, 2021

Sorry, provided the wrong git commit hash for the commit that introduced the issue. The correct commit is 21f7cf4.

@rotateright
Copy link
Contributor

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
index c8e5fdb458ff..1dabd676e16e 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -2499,7 +2499,7 @@ BoUpSLP::~BoUpSLP() {
"trying to erase instruction with users.");
Pair.getFirst()->eraseFromParent();
}

  • assert(!verifyFunction(*F, &dbgs()));
  • LLVM_DEBUG(verifyFunction(*F));
    }

Can you try it with your test file and see if that solves the slowdown?

@dyung
Copy link
Collaborator Author

dyung commented Jan 8, 2021

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?

@rotateright
Copy link
Contributor

Committed with:
https://reviews.llvm.org/rG267ff7901c74

@rnk
Copy link
Collaborator

rnk commented Jan 8, 2021

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:
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/Debug.h#L122

If I'm reading it correctly, LLVM_DEBUG only runs when the -debug LLVM option is set, and debug output from the relevant pass is enabled. This option is almost never enabled in the regular test suite, so with LLVM_DEBUG, this verification function is almost never called.

See also the bug I filed about verifyFunction being too slow:
#47056

@rotateright
Copy link
Contributor

Thanks for the explanation and bug link! The post-commit comments suggested my patch wasn't the right fix as well:
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20210104/869223.html

I'll post a new proposal on Phab, so we can reach a better conclusion.

@rotateright
Copy link
Contributor

I'll post a new proposal on Phab, so we can reach a better conclusion.

https://reviews.llvm.org/D94328

@rotateright
Copy link
Contributor

I went ahead and committed the expensive checks guard within SLP here because I think that's better than what I had before:
https://reviews.llvm.org/rG0aa75fb12faa

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?

@dyung
Copy link
Collaborator Author

dyung commented Jan 14, 2021

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).

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants