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

Remove deprecated LLVM-style inline assembly #92816

Merged
merged 14 commits into from
Jan 17, 2022
Merged

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jan 12, 2022

The llvm_asm! was deprecated back in #87590 1.56.0, with intention to remove
it once asm! was stabilized, which already happened in #91728 1.59.0. Now it
is time to remove llvm_asm! to avoid continued maintenance cost.

Closes #70173.
Closes #92794.
Closes #87612.
Closes #82065.

cc @rust-lang/wg-inline-asm

r? @Amanieu

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to rustc_codegen_gcc

cc @antoyo

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 12, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Jan 12, 2022

compiler/rustc_builtin_macros/src/asm.rs has a suggestion to use llvm_asm! if it detects the legacy syntax. I think this detection code should just be removed entirely since the renaming of the old asm! to llvm_asm! happened 2 years ago.

@rust-log-analyzer

This comment has been minimized.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 12, 2022

compiler/rustc_builtin_macros/src/asm.rs has a suggestion to use llvm_asm! if it detects the legacy syntax. I think this detection code should just be removed entirely since the renaming of the old asm! to llvm_asm! happened 2 years ago.

Thanks. Removed.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Jan 12, 2022

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jan 12, 2022

📌 Commit b085eb0 has been approved by Amanieu

@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 Jan 12, 2022
@bors
Copy link
Contributor

bors commented Jan 17, 2022

⌛ Testing commit b085eb0 with merge a34c079...

@bors
Copy link
Contributor

bors commented Jan 17, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing a34c079 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2022
@bors bors merged commit a34c079 into rust-lang:master Jan 17, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 17, 2022
@tmiasko tmiasko deleted the rm-llvm-asm branch January 17, 2022 13:24
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a34c079): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -7.5% on full builds of keccak check)
  • Moderate regression in instruction counts (up to 1.0% on incr-unchanged builds of ctfe-stress-4 check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-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 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

@rustbot rustbot added the perf-regression Performance regression. label Jan 17, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jan 17, 2022

Those are nice perf improvements!

@Amanieu
Copy link
Member

Amanieu commented Jan 17, 2022

I was honestly not expecting that. It seems that MIR borrowck somehow got a speedup, most likely due to the removal of MutateMode?

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 17, 2022

In keccak-Chec-Full perf difference seems incidental, code generated for a very hot loop in iterate_to_fixpoint improved quite a bit):

--------------------------------------------------------------------------------
Ir             
--------------------------------------------------------------------------------
-1,715,117,584  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir             file:function
--------------------------------------------------------------------------------
 -596,852,099  ???:<rustc_mir_dataflow::framework::engine::Engine<rustc_mir_dataflow::impls::MaybeInitializedPlaces>>::iterate_to_fixpoint
 -596,805,748  ???:<rustc_mir_dataflow::framework::engine::Engine<rustc_mir_dataflow::impls::MaybeUninitializedPlaces>>::iterate_to_fixpoint
 -523,105,586  ???:<rustc_mir_dataflow::framework::engine::Engine<rustc_mir_dataflow::impls::EverInitializedPlaces>>::iterate_to_fixpoint

This seems more or less what happened in a different PR (similar diff / codegen change) https://perf.rust-lang.org/compare.html?start=38c22af0153cf8f920c01ef04493e8878401fd18&end=8c4a951b919d548bba16b6f209ed05b1647eba2c.

cc @oli-obk I suspect that in #92928 & #93007 you might be chasing a regression that is no longer there.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 27, 2022
Remove deprecated LLVM-style inline assembly

The `llvm_asm!` was deprecated back in rust-lang#87590 1.56.0, with intention to remove
it once `asm!` was stabilized, which already happened in rust-lang#91728 1.59.0. Now it
is time to remove `llvm_asm!` to avoid continued maintenance cost.

Closes rust-lang#70173.
Closes rust-lang#92794.
Closes rust-lang#87612.
Closes rust-lang#82065.

cc `@rust-lang/wg-inline-asm`

r? `@Amanieu`
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 23, 2022
Remove deprecated LLVM-style inline assembly

The `llvm_asm!` was deprecated back in rust-lang#87590 1.56.0, with intention to remove
it once `asm!` was stabilized, which already happened in rust-lang#91728 1.59.0. Now it
is time to remove `llvm_asm!` to avoid continued maintenance cost.

Closes rust-lang#70173.
Closes rust-lang#92794.
Closes rust-lang#87612.
Closes rust-lang#82065.

cc `@rust-lang/wg-inline-asm`

r? `@Amanieu`
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Mar 30, 2022
Remove deprecated LLVM-style inline assembly

The `llvm_asm!` was deprecated back in rust-lang#87590 1.56.0, with intention to remove
it once `asm!` was stabilized, which already happened in rust-lang#91728 1.59.0. Now it
is time to remove `llvm_asm!` to avoid continued maintenance cost.

Closes rust-lang#70173.
Closes rust-lang#92794.
Closes rust-lang#87612.
Closes rust-lang#82065.

cc `@rust-lang/wg-inline-asm`

r? `@Amanieu`
drmorr0 added a commit to drmorr0/rust that referenced this pull request Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
8 participants