Reapply "cg_llvm: `fewer_names` in `uncached_llvm_type`" #88449

erikdesjardins · 2021-08-29T00:36:31Z

This reapplies #76030, which was reverted by #80122.

The underlying LLVM issue (https://bugs.llvm.org/show_bug.cgi?id=48340, per #79564 (comment)) was fixed in LLVM 13 by https://reviews.llvm.org/D91250 (and related patches), so we should be able to reland the original change.

I successfully ran the repro steps from #79564 (comment) using a rustc built with this change.

We should do another perf run to make sure this still has the same effect.

cc @davidtwco

This reverts commit 88dc58f.

rust-highfive · 2021-08-29T00:36:34Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

tmiasko · 2021-08-29T08:26:14Z

There was a report of a performance regression in #79246 that was eventually bisected to changes from #76030. Could you verify that this is no longer an issue?

It seems to me that issue was not merely incidental, but more fundamentally caused by the use of unnamed types. In terms of LLVM IR linked for LTO, one difference was quite apparent: the unnamed types, unlike named types previously, weren't being unified.

EDIT:

The original reproducer marmeladema/bitvec-perf-regression@684c72b still shows a regression when compiling with fat LTO. The regression is mostly a result of BitVec::push no longer being inlined into its only caller. It appears that for some reason the callsite doesn't receive large bonus from inlining the only call to the function with internal linkage.

nikic · 2021-08-29T09:09:01Z

@bors try @rust-timer queue

rust-timer · 2021-08-29T09:09:02Z

Awaiting bors try build completion.

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

bors · 2021-08-29T09:09:12Z

⌛ Trying commit dc905e4 with merge 71e222956789b7e8cb98a8925927a40b52ad9fe5...

bors · 2021-08-29T10:28:13Z

☀️ Try build successful - checks-actions
Build commit: 71e222956789b7e8cb98a8925927a40b52ad9fe5 (71e222956789b7e8cb98a8925927a40b52ad9fe5)

rust-timer · 2021-08-29T10:28:15Z

Queued 71e222956789b7e8cb98a8925927a40b52ad9fe5 with parent 757a65b, future comparison URL.

rust-timer · 2021-08-29T13:41:55Z

Finished benchmarking try commit (71e222956789b7e8cb98a8925927a40b52ad9fe5): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

Very large improvement in instruction counts (up to -61.4% on incr-patched: println builds of webrender-wrench)

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.

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

nagisa · 2021-08-30T10:51:38Z

@tmiasko given that the reproducer is a closed-source project, I don't see a good way for us to verify this by ourselves. Could @marmeladema try their project with this PR?

marmeladema · 2021-08-30T11:32:08Z

Is there an easy way for me to try a compiled version of rustc with this PR?

Urgau · 2021-08-30T11:47:32Z

Is there an easy way for me to try a compiled version of rustc with this PR?

I think you can use the rustup-toolchain-install-master tool to get the complete toolchain from a bors build. You just need to invoke it like this (where 71e2229 is from the latest bors build):

$ rustup-toolchain-install-master 71e222956789b7e8cb98a8925927a40b52ad9fe
$ rustc +71e222956789b7e8cb98a8925927a40b52ad9fe -vV

nagisa · 2021-08-30T11:48:06Z

The components are uploaded to an S3 bucket so you could reconstruct a toolchain from these, but its going to be a manual process. The bucket with the artifacts is at https://s3-us-west-1.amazonaws.com/rust-lang-ci2. The file locations are from the build logs:

Attempting with retry: aws s3 cp --no-progress --recursive --acl public-read /tmp/tmp.SswmvqxEIc s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5
upload: ../../../../../tmp/tmp.SswmvqxEIc/image-dist-x86_64-linux.txt to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/image-dist-x86_64-linux.txt
upload: ../../../../../tmp/tmp.SswmvqxEIc/cpu-dist-x86_64-linux.csv to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/cpu-dist-x86_64-linux.csv
upload: ../../../../../tmp/tmp.SswmvqxEIc/build-manifest-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/build-manifest-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/clippy-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/clippy-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/miri-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/miri-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-analysis-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-analysis-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-demangler-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-demangler-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/cargo-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/cargo-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-analyzer-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-analyzer-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/reproducible-artifacts-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/reproducible-artifacts-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/llvm-tools-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/llvm-tools-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-src-nightly.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-src-nightly.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-docs-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-docs-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rustc-docs-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rustc-docs-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rustc-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rustc-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rust-nightly-x86_64-unknown-linux-gnu.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-nightly-x86_64-unknown-linux-gnu.tar.xz
upload: ../../../../../tmp/tmp.SswmvqxEIc/rustc-nightly-src.tar.xz to s3://rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rustc-nightly-src.tar.xz

So for example https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-nightly-x86_64-unknown-linux-gnu.tar.xz should give you the same artifact as described in https://forge.rust-lang.org/infra/other-installation-methods.html#standalone-installers

(or what Urgau said ^^)

tmiasko · 2021-08-30T11:51:16Z

One still unfixed issue in LLVM exposed by this change https://bugs.llvm.org/show_bug.cgi?id=51667.

marmeladema · 2021-09-01T19:57:41Z

Ok, so I managed to install the toolchain from that PR:

$ rustup-toolchain-install-master -- 71e222956789b7e8cb98a8925927a40b52ad9fe5
detecting the channel of the `71e222956789b7e8cb98a8925927a40b52ad9fe5` toolchain...
downloading <https://ci-artifacts.rust-lang.org/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz>...
53.12 MB / 53.12 MB [====================================================================================================================================================================================================================] 100.00 % 9.91 MB/s 
downloading <https://ci-artifacts.rust-lang.org/rustc-builds/71e222956789b7e8cb98a8925927a40b52ad9fe5/rust-std-nightly-x86_64-unknown-linux-gnu.tar.xz>...
23.39 MB / 23.39 MB [====================================================================================================================================================================================================================] 100.00 % 8.31 MB/s 
toolchain `71e222956789b7e8cb98a8925927a40b52ad9fe5` is successfully installed!

I managed to compile my reproduction code:

$ cargo +71e222956789b7e8cb98a8925927a40b52ad9fe5 build --release
   Compiling libc v0.2.80
   Compiling serde v1.0.117
   Compiling ryu v1.0.5
   Compiling serde_json v1.0.59
   Compiling either v1.6.1
   Compiling itoa v0.4.6
   Compiling radium v0.3.0
   Compiling bitvec v0.17.4
   Compiling perf-event-open-sys v1.0.1
   Compiling perf-event v0.4.5
   Compiling bitvec-perf-regression v0.1.0 (/home/adema/code/bitvec-perf-regression)
    Finished release [optimized + debuginfo] target(s) in 10.44s

And running it shows this PR re-introduces the regression, and it's even worse than with version 1.48.0:

$ cargo +71e222956789b7e8cb98a8925927a40b52ad9fe5 run --release -- results.json
    Finished release [optimized + debuginfo] target(s) in 0.01s
     Running `target/release/bitvec-perf-regression results.json`
instructions:u = 1629821237
Regression of about 28% detected!

With current stable version (1.54.0), here is what you get:

$ cargo +stable run --release -- results.json
    Finished release [optimized + debuginfo] target(s) in 0.03s
     Running `target/release/bitvec-perf-regression results.json`
instructions:u = 1323627700

I also tried with current nightly rustc 1.56.0-nightly (29ef6cf 2021-08-31):

$ cargo +nightly run --release -- results.json
    Finished release [optimized + debuginfo] target(s) in 0.01s
     Running `target/release/bitvec-perf-regression results.json`
instructions:u = 1289248527

nagisa · 2021-09-05T14:38:01Z

@marmeladema is there any way we could obtain a self-contained reproducer for this problem?

marmeladema · 2021-09-05T14:46:48Z

I cannot really minimized it more and I don't know myself what the exact problem is.
However I thought that @tmiasko looked at my reproduction code and opened an llvm issue so I thought they had understood the problem?

nagisa · 2021-09-05T14:47:51Z

Ah, I didn't see the edit above from @tmiasko's post. Thank you for taking the time to respond!

nagisa · 2021-09-05T14:55:32Z

@erikdesjardins With the situation as it is right now, I do not believe we should merge this still. Inlining failing to do its job has potentially far-reaching consequences, which I'm not sure we can afford for the benefits that fewer_names brings us. Lets revisit this when the relevant issues (https://bugs.llvm.org/show_bug.cgi?id=51667) in LLVM are addressed.

erikdesjardins · 2021-09-05T17:04:57Z

Ah, I was just about to fix it: https://reviews.llvm.org/D109294

...although I suppose it's not worth backporting, so we'll have to wait for LLVM 14 anyways

Revert "Revert "cg_llvm: fewer_names in uncached_llvm_type""

dc905e4

This reverts commit 88dc58f.

rust-highfive assigned nagisa Aug 29, 2021

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 29, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 29, 2021

nagisa closed this Sep 5, 2021

erikdesjardins deleted the fewernames branch February 18, 2022 02:34

erikdesjardins mentioned this pull request Feb 18, 2022

Reapply cg_llvm: fewer_names in uncached_llvm_type #94107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reapply "cg_llvm: `fewer_names` in `uncached_llvm_type`" #88449

Reapply "cg_llvm: `fewer_names` in `uncached_llvm_type`" #88449

erikdesjardins commented Aug 29, 2021

rust-highfive commented Aug 29, 2021

tmiasko commented Aug 29, 2021 •

edited

Loading

nikic commented Aug 29, 2021

rust-timer commented Aug 29, 2021

bors commented Aug 29, 2021

bors commented Aug 29, 2021

rust-timer commented Aug 29, 2021

rust-timer commented Aug 29, 2021

nagisa commented Aug 30, 2021

marmeladema commented Aug 30, 2021

Urgau commented Aug 30, 2021

nagisa commented Aug 30, 2021 •

edited

Loading

tmiasko commented Aug 30, 2021

marmeladema commented Sep 1, 2021 •

edited

Loading

nagisa commented Sep 5, 2021

marmeladema commented Sep 5, 2021

nagisa commented Sep 5, 2021

nagisa commented Sep 5, 2021

erikdesjardins commented Sep 5, 2021

Reapply "cg_llvm: fewer_names in uncached_llvm_type" #88449

Reapply "cg_llvm: fewer_names in uncached_llvm_type" #88449

Conversation

erikdesjardins commented Aug 29, 2021

rust-highfive commented Aug 29, 2021

tmiasko commented Aug 29, 2021 • edited Loading

nikic commented Aug 29, 2021

rust-timer commented Aug 29, 2021

bors commented Aug 29, 2021

bors commented Aug 29, 2021

rust-timer commented Aug 29, 2021

rust-timer commented Aug 29, 2021

nagisa commented Aug 30, 2021

marmeladema commented Aug 30, 2021

Urgau commented Aug 30, 2021

nagisa commented Aug 30, 2021 • edited Loading

tmiasko commented Aug 30, 2021

marmeladema commented Sep 1, 2021 • edited Loading

nagisa commented Sep 5, 2021

marmeladema commented Sep 5, 2021

nagisa commented Sep 5, 2021

nagisa commented Sep 5, 2021

erikdesjardins commented Sep 5, 2021

Reapply "cg_llvm: `fewer_names` in `uncached_llvm_type`" #88449

Reapply "cg_llvm: `fewer_names` in `uncached_llvm_type`" #88449

tmiasko commented Aug 29, 2021 •

edited

Loading

nagisa commented Aug 30, 2021 •

edited

Loading

marmeladema commented Sep 1, 2021 •

edited

Loading