-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
[CMake][PGO] Build Sema.cpp to generate profdata for PGO builds #77347
Open
tstellar
wants to merge
7
commits into
llvm:main
Choose a base branch
from
tstellar:check-clang-pgo
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+51
−9
Open
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
127e2ae
[CMake][PGO] Use check-clang target to generate profdata for PGO builds
tstellar 4f77345
Fix python formatting
tstellar d330bff
Build Sema.cpp to generate profile data instead of using check-clang and
tstellar 9b6543d
Update documentatin
tstellar adcbfc0
Fix documentation build
tstellar 8b8837a
Fix documentation
tstellar 44aca59
Use check-cxx to generate profile data instead of Sema.cpp
tstellar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be really fragile. The paths of object file outputs are an implementation detail of CMake that has changed in the past and could change in the future, also I think only the makefile and ninja generator actually export those as buildable targets.
Beyond that I also have a philosophical problem with the approach of using living clang sources as training data. I think this can result in unexplainable or difficult to diagnose differences in the efficacy of PGO because as you change the compiler you're also innately changing the training data. A potential better solution would be to have a preprocessed Sema.cpp from a recent LLVM release that we check in as a stable copy. This would be similar to how the GCC compile-time benchmark works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the library target
clangSema
instead of the individual file?The first thing I tried to do was add a preprocessed version of CommentSema.cpp, but the file was 10MB, and I thought that might be too big. Sema.cpp preprocessed is 5MB, which is smaller but still pretty big. I would be OK with this approach if we think it's OK to add a big file like this to the repo.
I'm also not concerned about efficacy of PGO, because building Sema.cpp is just the default. I really want there to be a good default experience for users, so that they can use the PGO cache files and things will 'just work'. This patch also adds a configuration option for advanced users that need something more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the PGO use case is already an advanced-user feature. It's even documented in the "Advanced Builds" documentation.
If we want some more complex C++ sources there are some samples we could look at, like Kaleidoscope. The Kaleidoscope chapter 2 source builds without dependencies, and the other ones are also pretty simple to just compile.
If you're not concerned about the efficacy of the PGO build, I'm not sure why the existing single hello_world is a bad default. It isn't great but it is stable and something. If you want something that gives better performance then you must care about the efficacy, and I think you need to be somewhat concerned about the stability of those results, otherwise what's the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about the efficacy if it is 0%, but not really concerned if it bounces around between say 10 and 25 % based on updates to the file we are using to train.
I tested with the existing default (hello_world) and I didn't see any performance improvements from that. We also did some manual builds (not using the 2-stage PGO cache) and could not get any performance gains for using a hello world source. How certain are we that hello_world is enough to get performance improvements from PGO? Has anyone tested this recently? I will re-run my tests just to double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw small gains when I added it, but that was years ago. My confidence in it having any meaningful gain today is close to 0. I still think using training sources that have no stability is a bad idea. I'd feel much more comfortable using the LLVM examples or some other coding samples as testing sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea I'd like explore is using libc++ (and perhaps also libc) test suite for training. It consists of self-contained test programs which should sufficiently exercise various aspects of C/C++ frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the patch to use the check-cxx target to generate the profile data. The gains (~20%) are less than with Sema.cpp (~34%) but still pretty good. The only issue is that the profile data comes from building libcxx as well, so it's not just from the test suite.
It may be possible to get test data from just running the test suite, but that might be a lot more complicated to set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrhosek are you suggesting to use libc++ from in-tree or from a fixed and already released version as @llvm-beanz has suggested here? He hasn't suggested to use libc++ but a recent LLVM release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tstellar For the numbers you reported (~34% and ~20%), what did you use to collect those numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was compiling just a single file (SemaChecking.cpp).