Upgrade to LLVM 13 by nikic · Pull Request #87570 · rust-lang/rust · GitHub

Copy link

Contributor

nikic commented on Jul 28

rust-timer commented on Jul 28

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented on Jul 28

Trying commit ce064a2 with merge 450db03...

bors commented on Jul 28

Try build successful - checks-actions
Build commit: 450db03 (450db03509584046c77604194f41a448ac97e1c5)

rust-timer commented on Jul 28

rust-timer commented on Jul 29

Finished benchmarking try commit (450db03): comparison url.

Summary: This change led to significant mixed results in compiler performance.

• Very large regression in instruction counts (up to 31.7% on incr-unchanged builds of helloworld-opt)
• Very large improvement in instruction counts (up to -10.5% on incr-unchanged builds of helloworld-check)

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

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this 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-review -S-waiting-on-perf +perf-regression

nikic commented on Jul 29

It looks like the large regressions at the start are all slowdowns in run_linker, so this might be related to the missing llvmbc section flag.

hkratz commented on Jul 29

It looks like the large regressions at the start are all slowdowns in run_linker, so this might be related to the missing llvmbc section flag.

Seems like it. The resulting binaries do get bloated with bitcode.

The resulting stripped binary of compiling the helloworld perf test...

with Rust nightly 2021-07-28:

rustc -C opt-level=3 src/main.rs;strip main;ls -l main
-rwxr-xr-x 1 hans hans 272656 Jul 29 09:54 main

with LLVM 13:

~/dev/rust/build/x86_64-unknown-linux-gnu/stage2/bin/rustc -C opt-level=3 src/main.rs;strip main;ls -l main
-rwxr-xr-x 1 hans hans 6384232 Jul 29 10:07 main

Just backing out the hack and reverting the LLVM commit to allow the SHF_EXCLUDE flag on .llvmbc again works and makes it better:

~/dev/rust/build/x86_64-unknown-linux-gnu/stage2/bin/rustc -C opt-level=3 src/main.rs;strip main;ls -l main
-rwxr-xr-x 1 hans hans 309520 Jul 29 09:56 main

nikic commented on Jul 29

Weird issue encountered via the pgo-branch-weights test:

fn main() {
    println!("{:?}", std::env::args());
}

Running rustc test.rs && ./test works fine. Running rustc -Clink-args=-fuse-ld=gold test.rs && ./test gives empty arguments.

nikic commented 29 days ago

Just backing out the hack and reverting the LLVM commit to allow the SHF_EXCLUDE flag on .llvmbc again works and makes it better:

~/dev/rust/build/x86_64-unknown-linux-gnu/stage2/bin/rustc -C opt-level=3 src/main.rs;strip main;ls -l main
-rwxr-xr-x 1 hans hans 309520 Jul 29 09:56 main

Unfortunately, it seems like this doesn't actually work: Due to https://reviews.llvm.org/D100944 this no longer modifies the section flags on .llvmbc and will instead emit first an empty .llvmbc section with EXCLUDE, and then the actual .llvmbc section with ALLOC. LLVM is then no longer able to find the bitcode.

nikic commented 29 days ago

The good news is that reverting (just) https://reviews.llvm.org/D100944 does fix the llvmbc issue.

Running rustc test.rs && ./test works fine. Running rustc -Clink-args=-fuse-ld=gold test.rs && ./test gives empty arguments.

It looks like something is going wrong with .init_array:

objdump -t test_ld | grep init_array
0000000000048250 l    d  .init_array	0000000000000000              .init_array
0000000000048258 l     O .init_array	0000000000000000              __frame_dummy_init_array_entry
0000000000048250 l     O .init_array	0000000000000008              _ZN3std3sys4unix4args3imp15ARGV_INIT_ARRAY17hb13aa0f306b867eeE
0000000000048260 l       .init_array	0000000000000000              __init_array_end
0000000000048250 l       .init_array	0000000000000000              __init_array_start

objdump -t test_gold | grep init_array
000000000004a218 l     O .init_array	0000000000000000              __frame_dummy_init_array_entry
000000000004c728 l     O .init_array	0000000000000008              _ZN3std3sys4unix4args3imp15ARGV_INIT_ARRAY17hb13aa0f306b867eeE
000000000004a218 l       .init_array	0000000000000000              .hidden __init_array_start
000000000004a220 l       .init_array	0000000000000000              .hidden __init_array_end

Notably, with ld we have ARGV_INIT_ARRAY sitting between __init_array_start and __init_array_end. With gold it's located after __init_array_end.

nikic commented 29 days ago

rust-timer commented 29 days ago

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented 29 days ago

Trying commit 5f2b5c7 with merge 0983094...

bors commented 29 days ago

Try build successful - checks-actions
Build commit: 0983094 (0983094463497eec22d550dad25576a894687002)

rust-timer commented 29 days ago

rust-timer commented 29 days ago

Finished benchmarking try commit (0983094): comparison url.

Summary: This change led to significant mixed results in compiler performance.

• Very large improvement in instruction counts (up to -10.4% on incr-unchanged builds of helloworld-check)
• Large regression in instruction counts (up to 9.6% on full builds of match-stress-enum-check)

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

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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this 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-review -S-waiting-on-perf +perf-regression

nikic commented 27 days ago

Still rather stumped on the gold issue and don't really know how to debug it.

The best lead I have right now is that there are now some adjacent anon globals in the .data.rel.ro section, which were not present previously. Extracting the bitcode from the stdlib rlib, the relevant difference seems to be that a number of panic message constants are now hidden unnamed_addr constant, while they were previously private unnamed_addr constant. This means that these symbols now have external linkage, which they shouldn't have.

tmiasko commented 21 days ago •

Running rustc test.rs && ./test works fine. Running rustc -Clink-args=-fuse-ld=gold test.rs && ./test gives empty arguments.

#[used] attribute now results in SHF_GNU_RETAIN / R flag being set https://reviews.llvm.org/D97448, which seems to be causing the issue when linking with gold < binutils 2.36.

nikic commented 21 days ago

@tmiasko Ooh, nice find!

As suggested in the differential, we might want to switch to llvm.compiler.used then, as all our docs on #[used] make it abundantly clear that the attribute is only supposed to ensure that the symbol makes it into the object file, but the linker is still allowed to drop it.

This comment has been hidden.

bors commented 8 days ago

Test failed - checks-actions

nikic commented 8 days ago

@__llvm_profile_runtime_user is getting placed in @llvm.compiler.used rather than @llvm.used on Windows as well, so drop the separate expectation for that...

@bors r=nagisa

bors commented 8 days ago

Commit 306259c has been approved by nagisa

bors commented 8 days ago

Testing commit 306259c with merge db002a0...

bors commented 8 days ago

Test successful - checks-actions
Approved by: nagisa
Pushing db002a0 to master...

This has reduced by about 20% the number of array bound panics (and other similar panic) in my binaries (despite the final binary size is few percent bigger), this is a significant (good) change, I don't yet know what caused it but I'll try to understand it.

adrian17 commented 7 days ago

despite the final binary size is few percent bigger

I also observe 1-3% size increases in final binaries (including wasm); not sure if that is to be expected, or warrants an issue.

not sure if that is to be expected, or warrants an issue.

In my opinion this kind of slow little change is better tracked with specialized efforts (like the one for the compiler performance).

Upgrading to LLVM changes the profiling data format incompatibly compared to LLVM 12, breaking my CI/CD code coverage jobs. Thus nightly users doing source-based code coverage measurement LLVM 13's profiling tools. However, LLVM 13 hasn't been released yet, so it looks like people need to use https://github.com/llvm/llvm-project/releases/tag/llvmorg-13.0.0-rc1 for now.

We ship the necessary tools as part of llvm-tools-preview as far as I know, so I believe that should always work.

We ship the necessary tools as part of llvm-tools-preview as far as I know, so I believe that should always work.

Thanks for that pointer!

If you are building non-Rust code then you'll also need LLVM-13 compatible tools to use those new tools, e.g. clang-13.