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 AutoFDO. #87918

Merged
merged 1 commit into from
Oct 8, 2021
Merged

Enable AutoFDO. #87918

merged 1 commit into from
Oct 8, 2021

Conversation

mikebenfield
Copy link
Contributor

This largely involves implementing the options debug-info-for-profiling
and profile-sample-use and forwarding them on to LLVM.

AutoFDO can be used on x86-64 Linux like this:
rustc -O -Clink-arg='Wl,--no-rosegment' -Cdebug-info-for-profiling main.rs -o main
perf record -b ./main
create_llvm_prof --binary=main --out=code.prof
rustc -O -Cprofile-sample-use=code.prof main.rs -o main2

Now main2 will have feedback directed optimization applied to it.

The create_llvm_prof tool can be obtained from this github repository:
https://github.com/google/autofdo

The option -Clink-arg='Wl,--no-rosegment' is necessary to avoid lld
putting an extra RO segment before the executable code, which would make
the binary silently incompatible with create_llvm_prof.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2021
@rust-log-analyzer

This comment has been minimized.

@mikebenfield
Copy link
Contributor Author

I ran clang-format on the diff in PassWrapper.cpp, which reordered the includes. I can revert that part if desired.

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Aug 11, 2021

The option -Clink-arg='Wl,--no-rosegment' is necessary to avoid lld putting an extra RO segment before the executable code, which would make the binary silently incompatible with create_llvm_prof.

This will make read-only data executable, which depending on what program you have may or may not be a bad idea.

@mikebenfield
Copy link
Contributor Author

This will make read-only data executable, which depending on what program you have may or may not be a bad idea.

Indeed. Unfortunately it's the only way to deal with a current limitation of the create_llvm_prof tool, which can't deal with that RO segment. I've been told they plan to remove that limitation eventually, but that it won't happen in the immediate future.

@mikebenfield
Copy link
Contributor Author

Rebased, undid formatting in PassWrapper.cpp, fixed formatting in config.rs.

@mikebenfield
Copy link
Contributor Author

And fixed the commit message.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

I don't know anything about LLVM.
r? @nikic

@rust-highfive rust-highfive assigned nikic and unassigned cjgillot Aug 14, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@bors
Copy link
Contributor

bors commented Aug 26, 2021

☔ The latest upstream changes (presumably #88069) made this pull request unmergeable. Please resolve the merge conflicts.

@mikebenfield
Copy link
Contributor Author

A fix is in the works for the create_llvm_prof tool so it will no longer require --no-rosegment, so I removed that from the commit message.

@mikebenfield
Copy link
Contributor Author

@nikic Would you mind taking a look at this?

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.

This looks pretty straightforward and seems to mirror what clang does. Some high-level notes:

  • New options are always introduced as unstable -Z options first, and then stabilized as -C options. You should switch the new options to use -Z and open a tracking issue for this feature.
  • Not strictly required, but adding some basic docs on how to use this to src/doc/unstable-book/src/compiler-flags wouldn't hurt. You'll find profile.md and instrument_coverage.md there for related functionality. This could basically just be what you have in your PR description, maybe with an introduction on what this is actually about (it's easy to get lost in all these profiling, coverage and instrumentation options...)
  • It would be great to have some kind of testing for this. I see that the dependence on create_llvm_prof would make an end-to-end test inconvenient. If nothing else, we can test that -Cdebuginfo-for-profiling adds the function attribute using a test in src/test/codegen.

compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved
src/bootstrap/compile.rs Outdated Show resolved Hide resolved
compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/callee.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@mikebenfield
Copy link
Contributor Author

I made these into -Z options and added some documentation.

Testing seems more difficult. debug-info-for-profiling actually doesn't add a function attribute. profile-sample-use does, but it requires a profile, and I'm not sure whether it's wise to include a profile. Please let me know what you think.

@mikebenfield
Copy link
Contributor Author

@nikic Mind taking another look?

compiler/rustc_codegen_ssa/src/back/write.rs Outdated Show resolved Hide resolved
src/bootstrap/config.rs Outdated Show resolved Hide resolved
@mikebenfield
Copy link
Contributor Author

I addressed both recent comments.

@rust-log-analyzer

This comment has been minimized.

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.

Looks good, can you please squash the commits?

@mikebenfield
Copy link
Contributor Author

Squashed.

This largely involves implementing the options debug-info-for-profiling
and profile-sample-use and forwarding them on to LLVM.

AutoFDO can be used on x86-64 Linux like this:
rustc -O -Cdebug-info-for-profiling main.rs -o main
perf record -b ./main
create_llvm_prof --binary=main --out=code.prof
rustc -O -Cprofile-sample-use=code.prof main.rs -o main2

Now `main2` will have feedback directed optimization applied to it.

The create_llvm_prof tool can be obtained from this github repository:
https://github.com/google/autofdo

Fixes rust-lang#64892.
@mikebenfield
Copy link
Contributor Author

And fixed the commit message.

@nikic
Copy link
Contributor

nikic commented Oct 6, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2021

📌 Commit a17193d has been approved by nikic

@bors
Copy link
Contributor

bors commented Oct 6, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 7, 2021
Enable AutoFDO.

This largely involves implementing the options debug-info-for-profiling
and profile-sample-use and forwarding them on to LLVM.

AutoFDO can be used on x86-64 Linux like this:
rustc -O -Clink-arg='Wl,--no-rosegment' -Cdebug-info-for-profiling main.rs -o main
perf record -b ./main
create_llvm_prof --binary=main --out=code.prof
rustc -O -Cprofile-sample-use=code.prof main.rs -o main2

Now `main2` will have feedback directed optimization applied to it.

The create_llvm_prof tool can be obtained from this github repository:
https://github.com/google/autofdo

The option -Clink-arg='Wl,--no-rosegment' is necessary to avoid lld
putting an extra RO segment before the executable code, which would make
the binary silently incompatible with create_llvm_prof.
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 8, 2021
Enable AutoFDO.

This largely involves implementing the options debug-info-for-profiling
and profile-sample-use and forwarding them on to LLVM.

AutoFDO can be used on x86-64 Linux like this:
rustc -O -Clink-arg='Wl,--no-rosegment' -Cdebug-info-for-profiling main.rs -o main
perf record -b ./main
create_llvm_prof --binary=main --out=code.prof
rustc -O -Cprofile-sample-use=code.prof main.rs -o main2

Now `main2` will have feedback directed optimization applied to it.

The create_llvm_prof tool can be obtained from this github repository:
https://github.com/google/autofdo

The option -Clink-arg='Wl,--no-rosegment' is necessary to avoid lld
putting an extra RO segment before the executable code, which would make
the binary silently incompatible with create_llvm_prof.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2021
…ingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#87918 (Enable AutoFDO.)
 - rust-lang#88137 (On macOS, make strip="symbols" not pass any options to strip)
 - rust-lang#88772 (Fixed confusing wording on Result::map_or_else.)
 - rust-lang#89025 (Implement `#[link_ordinal(n)]`)
 - rust-lang#89082 (Implement rust-lang#85440 (Random test ordering))
 - rust-lang#89288 (Wrapper for `-Z gcc-ld=lld` to invoke rust-lld with the correct flavor)
 - rust-lang#89476 (Correct decoding of foreign expansions during incr. comp.)
 - rust-lang#89622 (Use correct edition for panic in [debug_]assert!().)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6c2d4bf into rust-lang:master Oct 8, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants