Avoid adding builtin functions to symbols.o

[DianQK]

We found performance regressions in #113923. The problem seems to be that --gc-sections does not remove these symbols. I tested that lld removes these symbols, but ld and gold do not.

I found that used adds symbols to symbols.o at

rust/compiler/rustc_codegen_ssa/src/back/linker.rs

Lines 1786 to 1791 in 3e202ea

	if info.level.is_below_threshold(export_threshold) || info.used {
	symbols.push((
	symbol_export::linking_symbol_name_for_instance_in_crate(tcx, symbol, cnum),
	info.kind,
	));
	}

.
The PR removes builtin functions.

Note that under LTO, ld still preserves these symbols. (lld will still remove them.)

The first commit also fixes #118559. But I think the second commit also makes sense.

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[DianQK]

r? @bjorn3

cc @nnethercote

We probably can perform a performance test.

[bjorn3]

symbols.o is necessary to ensure cyclic dependencies work correctly by forcing that the object file that defines them is pulled in and that the linker will consider this object file as a way to resolve uses of this symbol. This applies not just to linker-used symbols. If not including all these in symbols.o, we need some other way to make the linker behave in this way.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[DianQK]

symbols.o is necessary to ensure cyclic dependencies work correctly by forcing that the object file that defines them is pulled in and that the linker will consider this object file as a way to resolve uses of this symbol. This applies not just to linker-used symbols. If not including all these in symbols.o, we need some other way to make the linker behave in this way.

I moved #[used(compiler)] back to used. This should keep the original behavior.

[bjorn3]

I don't think we should check the name here. The compiler-builtins crate may have a different name. Maybe check for #[no_mangle] instead?

[DianQK]

Yeah, it's better.

[nnethercote]

It would be good to do a perf run on CI before this is merged, to confirm the expected perf effects.

[DianQK]

It would be good to do a perf run on CI before this is merged, to confirm the expected perf effects.

Could you do this for me? I don't have perf permissions.

[saethlin]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ca0738f with merge 3a9fb17...

[saethlin]

@DianQK You can always ask for a perf run https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/perf.20run which will probably be faster than hoping someone sees a GitHub comment. I mean, maybe it won't for you, since your work attracts so many eyes. But in general, the Zulip topic is pretty quck.

[DianQK]

@DianQK You can always ask for a perf run https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/perf.20run which will probably be faster than hoping someone sees a GitHub comment. I mean, maybe it won't for you, since your work attracts so many eyes. But in general, the Zulip topic is pretty quck.

Thanks. I saved this stream. :)

[bors]

☀️ Try build successful - checks-actions
Build commit: 3a9fb17 (3a9fb172c5d337b0436aa5f0785c97d3cbb973b9)

[rust-timer]

Finished benchmarking commit (3a9fb17): comparison URL.

Overall result: ✅ improvements - no action needed

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.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.8%	[-10.9%, -0.7%]	16
Improvements ✅ (secondary)	-5.0%	[-10.6%, -0.6%]	66
All ❌✅ (primary)	-5.8%	[-10.9%, -0.7%]	16

Max RSS (memory usage)

Results

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.5%	[1.4%, 3.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.8%, -1.5%]	2
Improvements ✅ (secondary)	-3.5%	[-3.5%, -3.5%]	1
All ❌✅ (primary)	0.2%	[-2.8%, 3.6%]	4

Cycles

Results

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)	-7.5%	[-11.7%, -1.3%]	13
Improvements ✅ (secondary)	-6.4%	[-11.6%, -1.2%]	52
All ❌✅ (primary)	-7.5%	[-11.7%, -1.3%]	13

Binary size

Results

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.0%	[0.0%, 0.0%]	27
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	4
Improvements ✅ (primary)	-3.1%	[-5.4%, -0.4%]	20
Improvements ✅ (secondary)	-5.1%	[-5.4%, -3.2%]	74
All ❌✅ (primary)	-1.3%	[-5.4%, 0.0%]	47

Bootstrap: 672.674s -> 673.359s (0.10%)
Artifact size: 314.14 MiB -> 313.88 MiB (-0.08%)

[pnkfelix]

@bors r+

[bors]

📌 Commit ca0738f has been approved by pnkfelix

It is now in the queue for this repository.

[bors]

⌛ Testing commit ca0738f with merge 503e129...

[bors]

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing 503e129 to master...

[rust-timer]

Finished benchmarking commit (503e129): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.6%	[-11.0%, -0.7%]	14
Improvements ✅ (secondary)	-5.0%	[-10.7%, -0.3%]	66
All ❌✅ (primary)	-6.6%	[-11.0%, -0.7%]	14

Max RSS (memory usage)

Results

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)	1.0%	[0.5%, 2.2%]	5
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-2.2%	[-4.1%, -0.5%]	6
Improvements ✅ (secondary)	-3.4%	[-3.7%, -3.2%]	2
All ❌✅ (primary)	-0.8%	[-4.1%, 2.2%]	11

Cycles

Results

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)	-7.1%	[-11.7%, -0.9%]	14
Improvements ✅ (secondary)	-6.5%	[-11.6%, -0.8%]	53
All ❌✅ (primary)	-7.1%	[-11.7%, -0.9%]	14

Binary size

Results

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.0%	[0.0%, 0.0%]	34
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	8
Improvements ✅ (primary)	-3.1%	[-5.5%, -0.4%]	20
Improvements ✅ (secondary)	-5.2%	[-5.5%, -3.3%]	74
All ❌✅ (primary)	-1.1%	[-5.5%, 0.0%]	54

Bootstrap: 675.63s -> 675.646s (0.00%)
Artifact size: 314.19 MiB -> 313.94 MiB (-0.08%)