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

Enable JumpTableToSwitch pass by default #82546

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

alexander-shaposhnikov
Copy link
Collaborator

Enable JumpTableToSwitch pass by default.

Test plan: ninja check-all

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@alexander-shaposhnikov alexander-shaposhnikov merged commit 1069823 into llvm:main Feb 22, 2024
4 checks passed
DavidSpickett added a commit that referenced this pull request Feb 26, 2024
This reverts commit 1069823.

This has caused second stage timeouts when building Flang on
AArch64:
https://lab.llvm.org/buildbot/#/builders/179/builds/9442
@DavidSpickett
Copy link
Collaborator

I've reverted this due to a timeout (which is set to 20 minutes) in the second stage of our AArch64 builders that include Flang.

https://lab.llvm.org/buildbot/#/builders/179/builds/9442

Reproducer: FlangRepro.zip

$ time /home/david.spickett/build-llvm-aarch64/bin/clang++   -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -fdiagnostics-color   -ffunction-sections -fdata-sections   -Wno-unused-command-line-argument   -Wno-gnu-line-marker -Wno-unused-value   -O3 -DNDEBUG -fno-exceptions   -funwind-tables -fno-rtti -UNDEBUG -std=c++17   -o /dev/null   -c /home/david.spickett/FlangRepro.cpp

Prior to this patch, an asserts enabled clang takes around 2 minutes. After the patch, I've seen it taking upwards of 20 minutes (could be even more, didn't stick around to find out).

Looking at the remaining processes at the end of the build, it's all flang stuff: files.log

They probably all include some header or use the same pattern that causes this.

@alexander-shaposhnikov
Copy link
Collaborator Author

alexander-shaposhnikov commented Feb 27, 2024

@DavidSpickett - many thanks for the report,
I've looked into the provided example and have done a few experiments.
It's kind of unfortunate in several aspects:

  1. The input is large (the decompressed source file is ~2.5MB) and on my MacBook it actually takes more than 3 minutes to compile.
  2. The optimization with the default settings triggers >11K times, there are many jump tables there, it looks like not all of them come from std::visit / std::variant, and they are long (90+% of them contain >= 9 functions, while the default threshold is 10).
  3. Out of curiosity I've tried to compile the same example with -mllvm -jump-table-to-switch-size-threshold=8 and the output object file is 2.2MB vs 2.4MB (when the optimization is turned off completely) - even though the pass has skipped most of the jump tables, it still has provided some profit. The compilation time was approximately the same.

Potential options:

  1. Adjust the default limit (e.g. to 8)
  2. For Flang set this limit in the build system
    @nikic - what do you think ?

p.s. I'll monitor the reports here, so in any case - huge thanks for the info, to the best of my knowledge this change has propagated through various CIs (not only at Google) and so far everything has been quite smooth (except this issue), but maybe it's too early to judge.

@nikic
Copy link
Contributor

nikic commented Feb 27, 2024

@alexander-shaposhnikov What is it that actually causes the compile-time increase in the end? Does the transform lead to catastrophic inlining or something?

Flang is a typical representative of the "gratuitous use of terribly designed STL primitives" style of C++ project, so if we're seeing compile-time explosion there, it seems quite likely that we'll also see it in other similar projects. Requiring a flag to work around that is not really acceptable.

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.

None yet

3 participants