The embedded bitcode should always be prepared for LTO/ThinLTO #133250

DianQK · 2024-11-20T14:24:34Z

There are currently two methods for generating bitcode that used for LTO. One method involves using -C linker-plugin-lto to emit object files as bitcode, which is the typical setting used by cargo. The other method is through -C embed-bitcode=yes.

When using with -C embed-bitcode=yes -C lto=no, we run a complete non-LTO LLVM pipeline to obtain bitcode, then the bitcode is used for LTO. We run the Call Graph Profile Pass twice on the same module.

This PR is doing something similar to LLVM's buildFatLTODefaultPipeline, obtaining the bitcode for embedding after running buildThinLTOPreLinkDefaultPipeline.

r? nikic

DianQK · 2024-11-20T14:25:35Z

@bors try @rust-timer queue

[WIP] The embedded bitcode should always be prepared for LTO/ThinLTO Fixes rust-lang#115344. Fixes rust-lang#117220. r? ghost

bors · 2024-11-20T14:26:46Z

⌛ Trying commit e91e0c7 with merge 5f7a0d8...

bors · 2024-11-20T14:41:04Z

💔 Test failed - checks-actions

DianQK · 2024-11-21T13:01:50Z

@bors try @rust-timer queue

bors · 2024-11-21T13:03:02Z

⌛ Trying commit fff2da3 with merge 5a6a7f3...

[WIP] The embedded bitcode should always be prepared for LTO/ThinLTO Fixes rust-lang#115344. Fixes rust-lang#117220. r? ghost

bors · 2024-11-21T14:46:22Z

☀️ Try build successful - checks-actions
Build commit: 5a6a7f3 (5a6a7f3cfdb88d3331274cd6b5d737c993dd8a98)

rust-timer · 2024-11-21T16:41:40Z

Finished benchmarking commit (5a6a7f3): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-2.4%, -0.5%]	4
All ❌✅ (primary)	0.5%	[0.4%, 0.7%]	2

Max RSS (memory usage)

Results (primary -0.2%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.0%, 2.3%]	2
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-5.0%	[-5.0%, -5.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-5.0%, 2.3%]	3

Cycles

Results (secondary -1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.3%, secondary -2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	4

Bootstrap: 796.195s -> 796.003s (-0.02%)
Artifact size: 335.92 MiB -> 335.87 MiB (-0.02%)

rustbot · 2024-11-24T14:06:12Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The embedded bitcode should always be prepared for LTO/ThinLTO Fixes rust-lang#115344. Fixes rust-lang#117220. There are currently two methods for generating bitcode that used for LTO. One method involves using `-C linker-plugin-lto` to emit object files as bitcode, which is the typical setting used by cargo. The other method is through `-C embed-bitcode=yes`. When using with `-C embed-bitcode=yes -C lto=no`, we run a complete non-LTO LLVM pipeline to obtain bitcode, then the bitcode is used for LTO. We run the Call Graph Profile Pass twice on the same module. This PR is doing something similar to LLVM's `buildFatLTODefaultPipeline`, obtaining the bitcode for embedding after running `buildThinLTOPreLinkDefaultPipeline`. r? nikic

bors · 2024-12-02T00:17:58Z

☀️ Try build successful - checks-actions
Build commit: 99acc6a (99acc6a24c3447932825ce66c54d7ae1a4b0499e)

rust-timer · 2024-12-02T01:34:21Z

Finished benchmarking commit (99acc6a): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.5%]	4
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	0.4%	[0.4%, 0.5%]	4

Max RSS (memory usage)

Results (primary -0.7%, secondary 2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.6%, 3.1%]	2
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	1

Cycles

Results (primary -1.3%, secondary -1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-1.3%	[-1.3%, -1.3%]	1

Binary size

Results (primary 0.3%, secondary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.1%, 0.7%]	58
Regressions ❌ (secondary)	0.9%	[0.0%, 5.3%]	21
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.1%]	8
Improvements ✅ (secondary)	-0.4%	[-1.7%, -0.3%]	38
All ❌✅ (primary)	0.3%	[-0.4%, 0.7%]	66

Bootstrap: 768.518s -> 768.387s (-0.02%)
Artifact size: 332.12 MiB -> 332.19 MiB (0.02%)

DianQK · 2024-12-03T01:10:31Z

I think this is basically ready for review. Some potential adjustments could be addressed in subsequent PRs:

(Possibly in this PR) Rename ThinBitcode to PreLtoBitcode?
Refactor the code related to emit_pre_lto_bc
Reuse the embed bitcode for --emit=llvm-bc when using -C linker-plugin-lto?

@rustbot review

DianQK · 2024-12-09T01:47:17Z

I think this is basically ready for review. Some potential adjustments could be addressed in subsequent PRs:

(Possibly in this PR) Rename ThinBitcode to PreLtoBitcode?

Refactor the code related to emit_pre_lto_bc

Reuse the embed bitcode for --emit=llvm-bc when using -C linker-plugin-lto?

@rustbot review

Hmm, when passing -Z emit-thin-lto=false, it's different bitcode between the embed bitcode and the pre-LTO bitcode. I think all changes can be discussed later.

DianQK · 2024-12-17T01:53:12Z

Two weeks ping

DianQK · 2024-12-30T10:17:23Z

Two weeks ping again @nikic

DianQK · 2025-01-07T12:04:24Z

ping
If there's anything that needs to be changed, please don't hesitate to let me know.

nikic

This generally looks reasonable to me.

Is writing the bitcode out into the incremental directory the usual thing to do, as opposed to keeping it in memory until it is embedded?

compiler/rustc_codegen_llvm/src/back/write.rs

compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp

nikic · 2025-01-07T16:07:13Z

compiler/rustc_session/src/config.rs

Do the changes here also enable --emit=thin-llvm-bc? If yes, is it stable or unstable?

I think we can only emit it under the -Csave-temps flag that I haven't implemented yet.

nikic · 2025-01-07T16:10:49Z

compiler/rustc_session/src/config.rs

@@ -508,6 +508,7 @@ impl FromStr for SplitDwarfKind {
 pub enum OutputType {
    Bitcode,
    ThinLinkBitcode,
+    ThinBitcode,


Please add doc comments to Bitcode, ThinLinkBitcode and ThinBitcode to explain what the difference between them is...

DianQK · 2025-01-08T14:24:37Z

I've rebased due to conflicts with #130060.

Is writing the bitcode out into the incremental directory the usual thing to do, as opposed to keeping it in memory until it is embedded?

I can put Option<ThinBuffer> in ModuleCodegen, but this might be a significant change. Perhaps I could use Option<Vec<u8>> first?

@rustbot author

bors · 2025-01-21T13:08:24Z

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

rustbot · 2025-02-06T14:18:52Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

DianQK · 2025-02-06T14:21:43Z

Sorry for the late update. Pre-linked bitcode no longer needs to be saved to a file now.

@rustbot review

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 20, 2024