Skip to content

Allow to disable thinLTO buffer to support lto-embed-bitcode lld feature - #98162

Merged
bors merged 1 commit into
rust-lang:masterfrom
nextsilicon:support_lto_embed_bitcode
Jul 21, 2022
Merged

Allow to disable thinLTO buffer to support lto-embed-bitcode lld feature#98162
bors merged 1 commit into
rust-lang:masterfrom
nextsilicon:support_lto_embed_bitcode

Conversation

@darkness-ai

@darkness-aidarkness-ai commented Jun 16, 2022

Copy link
Copy Markdown
Contributor

Hello
This change is to fix issue (#84395) in which passing "-lto-embed-bitcode=optimized" to lld when linking rust code via linker-plugin-lto doesn't produce the expected result.

Instead of emitting a single unified module into a llvmbc section of the linked elf, it emits multiple submodules.
This is caused because rustc emits the BC modules after running llvm createWriteThinLTOBitcodePass pass.
Which in turn triggers a thinLTO linkage and causes the said issue.

This patch allows via compiler flag (-Cemit-thin-lto=) to select between running createWriteThinLTOBitcodePass and createBitcodeWriterPass.
Note this pattern of selecting between those 2 passes is common inside of LLVM code.
The default is to match the old behavior.

@rustbotrustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 16, 2022
@rust-highfive

Copy link
Copy Markdown
Contributor

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

Please see the contribution instructions for more information.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2022
@lcnr

lcnr commented Jun 28, 2022

Copy link
Copy Markdown
Contributor

seems like i missed the notification for this PR 😅 sorry

r? rust-lang/compiler

@rust-highfiverust-highfive assigned davidtwco and unassigned lcnrJun 28, 2022
Comment threadcompiler/rustc_session/src/options.rs Outdated
@davidtwcodavidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2022
@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from 4cb2088 to cb61ea9CompareJuly 3, 2022 08:06
@rust-log-analyzer

This comment has been minimized.

@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from cb61ea9 to 725971aCompareJuly 3, 2022 08:35
@darkness-ai

Copy link
Copy Markdown
ContributorAuthor

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 3, 2022
@darkness-ai

Copy link
Copy Markdown
ContributorAuthor

@rustbot label -S-waiting-on-review +S-waiting-on-author

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022
@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from 725971a to d6ac2b6CompareJuly 3, 2022 10:54
@rust-log-analyzer

This comment has been minimized.

@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from d6ac2b6 to 87967e8CompareJuly 3, 2022 12:59
@darkness-ai

Copy link
Copy Markdown
ContributorAuthor

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 3, 2022
@bors

This comment was marked as resolved.

@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from 87967e8 to f9c1459CompareJuly 4, 2022 06:51

@davidtwcodavidtwco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think you could add a test case that uses this and -lto-embed-bitcode=optimized to confirm that it fixes the issue?

@davidtwcodavidtwco removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2022
@borsbors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 9, 2022
@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from 28b6f43 to 3858cddCompareJuly 9, 2022 11:33
@darkness-ai

Copy link
Copy Markdown
ContributorAuthor

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2022
@bors

bors commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

@darkness-ai: 🔑 Insufficient privileges: Not in reviewers

@davidtwco

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jul 11, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 3858cddaec9a7480b94bfa8432c9ed4df13b0478 has been approved by davidtwco

It is now in the queue for this repository.

@borsbors 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 Jul 11, 2022
Adding the option to control from rustc CLI
if the resulted ".o" bitcode module files are with
thinLTO info or regular LTO info.
Allows using "-lto-embed-bitcode=optimized" during linkage
correctly.
Signed-off-by: Ziv Dunkelman <ziv.dunkelman@nextsilicon.com>
@darkness-ai
darkness-aiforce-pushed the support_lto_embed_bitcode branch from 3858cdd to 724c912CompareJuly 14, 2022 19:22
@darkness-ai

Copy link
Copy Markdown
ContributorAuthor

@davidtwco saw PRs conflicting with this one got merged into master.
Pushed fixes for the mismatch.

@davidtwco

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jul 15, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 724c912 has been approved by davidtwco

It is now in the queue for this repository.

@bors

bors commented Jul 21, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 724c912 with merge 74f600b...

@bors

bors commented Jul 21, 2022

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 74f600b to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2022
@bors
bors merged commit 74f600b into rust-lang:masterJul 21, 2022
@rustbotrustbot added this to the 1.64.0 milestone Jul 21, 2022
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (74f600b): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: no relevant changes found
mean1maxcount2
Regressions 😿
(primary)
N/AN/A0
Regressions 😿
(secondary)
N/AN/A0
Improvements 🎉
(primary)
-0.4%-0.4%2
Improvements 🎉
(secondary)
N/AN/A0
All 😿🎉 (primary)-0.4%-0.4%2

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 😿 relevant regression found
mean1maxcount2
Regressions 😿
(primary)
N/AN/A0
Regressions 😿
(secondary)
5.7%5.7%1
Improvements 🎉
(primary)
-2.2%-2.2%1
Improvements 🎉
(secondary)
N/AN/A0
All 😿🎉 (primary)-2.2%-2.2%1

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1maxcount2
Regressions 😿
(primary)
1.8%1.8%1
Regressions 😿
(secondary)
N/AN/A0
Improvements 🎉
(primary)
N/AN/A0
Improvements 🎉
(secondary)
-2.5%-2.5%1
All 😿🎉 (primary)1.8%1.8%1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change23

  2. number of relevant changes23

@darkness-ai
darkness-ai deleted the support_lto_embed_bitcode branch July 26, 2022 07:51
This was referenced Feb 11, 2026
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 20, 2026
…uviper
Remove -Zemit-thin-lto flag
As far as I can tell it was introduced in rust-lang#98162 to allow fat LTO with `-Clinker-plugin-lto`. In rust-lang#136840 a change was made to automatically disable ThinLTO summary generation when `-Clinker-plugin-lto -Clto=fat` is used, so we can safely remove it.
Fixesrust-lang#152490
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Feb 20, 2026
…uviper
Remove -Zemit-thin-lto flag
As far as I can tell it was introduced in rust-lang#98162 to allow fat LTO with `-Clinker-plugin-lto`. In rust-lang#136840 a change was made to automatically disable ThinLTO summary generation when `-Clinker-plugin-lto -Clto=fat` is used, so we can safely remove it.
Fixesrust-lang#152490
rust-timer added a commit that referenced this pull request Feb 21, 2026
Rollup merge of #152527 - bjorn3:remove_z_emit_thin_lto, r=cuviper
Remove -Zemit-thin-lto flag
As far as I can tell it was introduced in #98162 to allow fat LTO with `-Clinker-plugin-lto`. In #136840 a change was made to automatically disable ThinLTO summary generation when `-Clinker-plugin-lto -Clto=fat` is used, so we can safely remove it.
Fixes#152490
github-actionsBot pushed a commit to rust-lang/miri that referenced this pull request Feb 22, 2026
Remove -Zemit-thin-lto flag
As far as I can tell it was introduced in rust-lang/rust#98162 to allow fat LTO with `-Clinker-plugin-lto`. In rust-lang/rust#136840 a change was made to automatically disable ThinLTO summary generation when `-Clinker-plugin-lto -Clto=fat` is used, so we can safely remove it.
Fixesrust-lang/rust#152490
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant 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

@darkness-ai@rust-highfive@lcnr@rust-log-analyzer@bors@davidtwco@rust-timer@bjorn3@rustbot