Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore #![no_builtins] crates participation in LTO. #113923

Merged
merged 10 commits into from
Dec 1, 2023

Conversation

DianQK
Copy link
Member

@DianQK DianQK commented Jul 21, 2023

After #113716, we can make #![no_builtins] crates participate in LTO again.

#![no_builtins] with LTO does not result in undefined references to the error. I believe this type of issue won't happen again.

Fixes #72140. Fixes #112245. Fixes #110606. Fixes #105734. Fixes #96486. Fixes #108853. Fixes #108893. Fixes #78744. Fixes #91158. Fixes rust-lang/cargo#10118. Fixes rust-lang/compiler-builtins#347.

The nightly-2023-07-20 version does not always reproduce problems due to changes in compiler-builtins, core, and user code. That's why this issue recurs and disappears.
Some issues were not tested due to the difficulty of reproducing them.

r? pnkfelix

cc @bjorn3 @japaric @alexcrichton @Amanieu

@rustbot rustbot added 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 Jul 21, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@DianQK
Copy link
Member Author

DianQK commented Jul 22, 2023

Removed opaque pointers for LLVM 14.

@DianQK
Copy link
Member Author

DianQK commented Jul 29, 2023

ping? @pnkfelix

@DianQK
Copy link
Member Author

DianQK commented Aug 10, 2023

Rebase and remove the DisableSimplifyLibCalls parameter.

@DianQK
Copy link
Member Author

DianQK commented Aug 15, 2023

@rustbot ready

@pnkfelix
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 17, 2023

📌 Commit c1ec76c has been approved by pnkfelix

It is now in the queue for this repository.

@bors bors 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 Aug 17, 2023
@bors
Copy link
Contributor

bors commented Aug 17, 2023

⌛ Testing commit c1ec76c with merge f4589297857113e2cc3f6437f112d567b4da3694...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 17, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2023
@DianQK
Copy link
Member Author

DianQK commented Aug 18, 2023

Based on the error message, I believe #78744, #91158 amd rust-lang/cargo#10118 are also not participating in the LTO issue.
But let me fix the no_builtins issue with wasm first.

It show

  (import "env" "__muloti4" (func $__muloti4 (type 0)))

@DianQK
Copy link
Member Author

DianQK commented Aug 18, 2023

@rustbot author

@rustbot rustbot 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 Aug 18, 2023
@DianQK
Copy link
Member Author

DianQK commented Aug 19, 2023

TL;DR
Using built-in functions like __multi3 does not generate calls during IR optimization.
This call is not generated until the LLVM code generation phase.
https://llvm.godbolt.org/z/aoMK4KTWT is an example.

I renamed wasm-spurious-import to wasm-builtins-import. Because we have found the essence of the problem, and it is the same issue that this PR is trying to solve. (That's why it magically disappeared.)
Now we use this test to check that the built-in functions are not missing.


An interesting attempt: I can get correct results using LLVM's LTO in lld.
I just need to change the main.main.7fc14dcaa7e3fa65-cgu.0.rcgu.o to main.main.7fc14dcaa7e3fa65-cgu.0.rcgu.lto.input.bc.

"lld" "-flavor" "wasm" "--rsp-quoting=posix" \
"--export" "multer" "--export=__heap_base" "--export=__data_end" \
"-z" "stack-size=1048576" "--stack-first" "--allow-undefined" "--fatal-warnings" "--no-demangle" "--no-entry" \
"main.main.7fc14dcaa7e3fa65-cgu.0.rcgu.lto.input.bc" \
"-o" "main.wasm" "--gc-sections" "--no-entry" "-O3"

I investigated along with this result why lld's LTO could be successful.
Then I found llvm/llvm-project@e06bac4.


Two issues in LLVM (or maybe not).

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8c2b577): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)
6.4% [0.8%, 12.4%] 17
Regressions ❌
(secondary)
5.3% [0.5%, 12.2%] 72
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 6.4% [0.8%, 12.4%] 17

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)
3.0% [1.5%, 4.5%] 2
Regressions ❌
(secondary)
3.1% [2.7%, 3.7%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-3.1%, -1.0%] 2
All ❌✅ (primary) 3.0% [1.5%, 4.5%] 2

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)
7.0% [1.7%, 13.4%] 17
Regressions ❌
(secondary)
6.7% [1.1%, 13.2%] 60
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 7.0% [1.7%, 13.4%] 17

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)
3.7% [0.4%, 5.8%] 24
Regressions ❌
(secondary)
5.4% [0.2%, 5.9%] 76
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.7% [0.4%, 5.8%] 24

Bootstrap: 673.541s -> 672.846s (-0.10%)
Artifact size: 313.37 MiB -> 313.94 MiB (0.18%)

@rustbot rustbot added the perf-regression Performance regression. label Dec 2, 2023
@bjorn3
Copy link
Member

bjorn3 commented Dec 2, 2023

It seems that linking has become 10-20ms slower and binaries ~260kb bigger. Maybe we no longer gc unused compiler builtins functions?

@nnethercote
Copy link
Contributor

Right, those compile-time regressions are really bad :( Lots of ~5% binary size increases, too. What can be done?

@bjorn3
Copy link
Member

bjorn3 commented Dec 2, 2023

I believe I saw something around symbols being marked as "used". If this is currently linker used, maybe we can downgrade it to compiler used?

@DianQK
Copy link
Member Author

DianQK commented Dec 2, 2023

Right, those compile-time regressions are really bad :( Lots of ~5% binary size increases, too. What can be done?

I can verify that this indeed preserves builtin functions.

@DianQK
Copy link
Member Author

DianQK commented Dec 2, 2023

It seems that linking has become 10-20ms slower and binaries ~260kb bigger. Maybe we no longer gc unused compiler builtins functions?

This should be the right way to go.

I believe I saw something around symbols being marked as "used". If this is currently linker used, maybe we can downgrade it to compiler used?

I will start debugging with your analysis this weekend. :)

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2023
Avoid adding builtin functions to `symbols.o`

We found performance regressions in rust-lang#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 https://github.com/rust-lang/rust/blob/3e202ead604be31f4c1a5798a296953d3159da7e/compiler/rustc_codegen_ssa/src/back/linker.rs#L1786-L1791.
The PR removes builtin functions.

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

The first commit also fixes rust-lang#118559. But I think the second commit also makes sense.
@rylev
Copy link
Member

rylev commented Dec 5, 2023

#118568 seems to reverse these perf regressions.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2023
Avoid adding builtin functions to `symbols.o`

We found performance regressions in rust-lang#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 https://github.com/rust-lang/rust/blob/3e202ead604be31f4c1a5798a296953d3159da7e/compiler/rustc_codegen_ssa/src/back/linker.rs#L1786-L1791.
The PR removes builtin functions.

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

The first commit also fixes rust-lang#118559. But I think the second commit also makes sense.
DianQK added a commit to DianQK/rust that referenced this pull request Jan 12, 2024
…to, r=pnkfelix"

This reverts commit 8c2b577, reversing
changes made to 9cf18e9.
DianQK added a commit to DianQK/rust that referenced this pull request Jan 12, 2024
…to, r=pnkfelix"

This reverts commit 8c2b577, reversing
changes made to 9cf18e9.
This was referenced Jan 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2024
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#119817 (Remove special-casing around `AliasKind::Opaque` when structurally resolving in new solver)
 - rust-lang#119819 (Check rust lints when an unknown lint is detected)
 - rust-lang#119872 (Give me a way to emit all the delayed bugs as errors (add `-Zeagerly-emit-delayed-bugs`))
 - rust-lang#119877 (Add more information to `visit_projection_elem`)
 - rust-lang#119884 (Rename `--env` option flag to `--env-set`)
 - rust-lang#119885 (Revert rust-lang#113923)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2024
Rollup merge of rust-lang#119885 - DianQK:revert-pr-113923, r=petrochenkov

Revert rust-lang#113923

Per [#t-compiler/meetings > [weekly] 2024-01-11](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-01-11) discussion, revert rust-lang#113923. Also revert associated rust-lang#118568.

The PR rust-lang#113923 causes the regression issue rust-lang#118609. We need more time to find a proper solution.

Discussions start at [412365838](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-01-11/near/412365838) and continue to [412369643](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-01-11/near/412369643).

Fixes rust-lang#118609.

r? compiler
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
bjorn3 added a commit to bjorn3/c-ward that referenced this pull request Feb 16, 2024
Since rust-lang/rust#113923 no_builtins crates
participate in LTO too.
sunfishcode pushed a commit to sunfishcode/c-ward that referenced this pull request Feb 17, 2024
Since rust-lang/rust#113923 no_builtins crates
participate in LTO too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment