Deprecate llvm_asm! #87590

Amanieu · 2021-07-29T15:06:05Z

We would like to remove llvm_asm! from the compiler once asm! is stabilized. This PR deprecates llvm_asm! to encourage any remaining users to migrate to asm! (or if asm! is not supported for their target, to add this support to rustc).

The only remaining user of llvm_asm! in the standard library was black_box, which has been rewritten to use volatile operations when asm! is not available on the current target.

cc @rust-lang/wg-inline-asm

cc @RalfJung for the changes to black_box which might affect Miri.

r? @nagisa

joshtriplett · 2021-07-29T15:57:10Z

For simplicity's sake, is there any reason not to just use volatile operations on all platforms, and drop the complex and duplicated cfg condition?

Amanieu · 2021-07-29T16:18:07Z

Using asm! can be slightly faster if the value is already in memory. Also it can be further optimized in the future as suggested here: #64102 (comment)

nagisa · 2021-07-29T16:32:49Z

library/core/src/hint.rs

+    // 1. passing the value to some external process
+    // 2. allowing that process to access any memory that the value may point to
+    // 3. reading a potentially modified value back
+    #[cfg(any(


My concern with this implementation is that once support for assembly is added for any of these architectures, it is trivial to miss updating this location.

Any way we can avoid this? We could make up an intrinsic for this, perhaps?

Instead of using the fallback for all other architectures, could we have an explicit list of architectures that do use the fallback?

Instead of using the fallback for all other architectures, could we have an explicit list of architectures that do use the fallback?

This will likely cause problems when people try to use a new architecture with a custom json target.

Any way we can avoid this? We could make up an intrinsic for this, perhaps?

We could have a target_has_asm cfg flag like we do for atomics. But it seems a bit excessive to have this just for black_box.

Having thought about this more, an intrinsic seems significantly more straightforward to me. But I guess I'm okay with this implementation too.

If we continue using cfgs here, consider cfg_if! maybe? It'll reduce the clutter here significantly I feel.

nagisa · 2021-07-29T16:33:53Z

I think the direction is reasonable. See the concern above, once its resolved and the tests are fixed, r=me.

joshtriplett · 2021-07-29T21:48:22Z

On Thu, Jul 29, 2021 at 11:19:22AM -0700, Amanieu wrote: We could have a `target_has_asm` cfg flag like we do for atomics. But it seems a bit excessive to have this just for `black_box`.

I think it might be worthwhile regardless, and I can easily imagine other uses of that as well. Alternatively, is the performance improvement of using `asm!` sufficiently large to be worth the maintenance headache? Using volatile everywhere would leave `black_box` still entirely usable for benchmarks.

Amanieu · 2021-07-30T14:32:52Z

The main use of black_box is in micro-benchmarking where you want it to have as little impact as possible on the benchmark. I would argue that performance is pretty important in this case.

There are more optimizations that can be done to black_box, such as passing the value directly in a register instead of just its address to avoid forcing a store to memory. However this is not the main point of this PR which is to deprecate llvm_asm! and remove its uses from the standard library.

RalfJung · 2021-07-30T15:22:55Z

cc @RalfJung for the changes to black_box which might affect Miri.

Looks good -- though Miri doesn't optimize, so skipping the function entirely like before would also work. No strong opinion either way.

joshtriplett · 2021-07-30T19:51:34Z

On Fri, Jul 30, 2021 at 07:33:03AM -0700, Amanieu wrote: The main use of `black_box` is in micro-benchmarking where you want it to have as little impact as possible on the benchmark. I would argue that performance is pretty important in this case.

Then I think, for maintainability reasons, it'd be worth adding a `cfg(target_has_asm)` or similar, even if it's internal-use-only for now. That long architecture list seems like a painful maintenance hazard.

bors · 2021-08-08T02:16:56Z

☔ The latest upstream changes (presumably #87772) made this pull request unmergeable. Please resolve the merge conflicts.

Implement `black_box` using intrinsic Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment). This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity. cc `@Amanieu` as this is related to inline assembly cc `@bjorn3` for rustc_codegen_cranelift changes cc `@RalfJung` as this affects MIRI r? `@nagisa` I suppose

Amanieu · 2021-08-11T23:31:22Z

Should be good to go, however this will conflict with #87916 due to the need for an allow(deprecated) for black_box.

Implement `black_box` using intrinsic Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment). This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity. cc `@Amanieu` as this is related to inline assembly cc `@bjorn3` for rustc_codegen_cranelift changes cc `@RalfJung` as this affects MIRI r? `@nagisa` I suppose

bors · 2021-08-13T00:06:38Z

☔ The latest upstream changes (presumably #87916) made this pull request unmergeable. Please resolve the merge conflicts.

nbdd0121 · 2021-08-14T03:07:14Z

src/test/ui/abi/abi-sysv64-register-usage.rs

@@ -7,6 +7,7 @@
 // ignore-aarch64

 #![feature(llvm_asm)]
+#![allow(deprecated)] // llvm_asm!


Shouldn't this test be modified to use asm! instead? This test isn't testing llvm_asm! itself

Perfectly fine to leave removal to a follow-up PR (potentially the same one that removes llvm_asm entirely.

nagisa · 2021-08-14T15:24:34Z

@bors r+

bors · 2021-08-14T15:24:35Z

📌 Commit ee5adcdf9199d94aabecea7cdf8515f2877bd45e has been approved by nagisa

bors · 2021-08-15T01:47:17Z

☔ The latest upstream changes (presumably #87581) made this pull request unmergeable. Please resolve the merge conflicts.

Amanieu · 2021-08-15T21:19:38Z

@bors r=nagisa

bors · 2021-08-15T21:19:39Z

📌 Commit 632a400 has been approved by nagisa

bors · 2021-08-15T21:22:05Z

⌛ Testing commit 632a400 with merge 2bd17c1...

bors · 2021-08-16T00:10:04Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 2bd17c1 to master...

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`

rust-highfive assigned nagisa Jul 29, 2021

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

This comment has been minimized.

Sign in to view

Amanieu force-pushed the deprecate_llvm_asm branch from 55e8423 to 1c95f1c Compare July 29, 2021 16:16

This comment has been minimized.

Sign in to view

nagisa reviewed Jul 29, 2021

View reviewed changes

nagisa mentioned this pull request Aug 4, 2021

global_llvm_asm! #87753

Closed

Amanieu mentioned this pull request Aug 4, 2021

Segfault when using llvm_asm memory constraint #87612

Closed

nagisa 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 8, 2021

nbdd0121 mentioned this pull request Aug 10, 2021

Implement black_box using intrinsic #87916

Merged

Amanieu force-pushed the deprecate_llvm_asm branch 2 times, most recently from 6eae2e0 to 53ab846 Compare August 11, 2021 14:45

This comment has been minimized.

Sign in to view

Pratyush mentioned this pull request Aug 11, 2021

Migrate away from llvm_asm to asm arkworks-rs/algebra#305

Closed

Amanieu force-pushed the deprecate_llvm_asm branch from 53ab846 to 4280a5e Compare August 11, 2021 23:26

Amanieu force-pushed the deprecate_llvm_asm branch from 4280a5e to ee5adcd Compare August 13, 2021 01:15

nbdd0121 reviewed Aug 14, 2021

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2021

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 15, 2021

Amanieu added 3 commits August 15, 2021 13:14

Deprecate llvm_asm!

4dd933c

Allow the use of the deprecated llvm_asm! in black_box

6fd4f34

Fix ui tests for llvm_asm! deprecation

632a400

Amanieu force-pushed the deprecate_llvm_asm branch from ee5adcd to 632a400 Compare August 15, 2021 12:27

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 15, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 16, 2021

bors merged commit 2bd17c1 into rust-lang:master Aug 16, 2021

rustbot added this to the 1.56.0 milestone Aug 16, 2021

This was referenced Aug 16, 2021

Remove all json handling from rustc_serialize #85993

Merged

Upgrade to LLVM 13 #87570

Merged

tmiasko mentioned this pull request Jan 12, 2022

Remove deprecated LLVM-style inline assembly #92816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate llvm_asm! #87590

Deprecate llvm_asm! #87590

Amanieu commented Jul 29, 2021

This comment has been minimized.

joshtriplett commented Jul 29, 2021

Amanieu commented Jul 29, 2021

This comment has been minimized.

nagisa Jul 29, 2021

RalfJung Jul 29, 2021

Amanieu Jul 29, 2021

nagisa Aug 1, 2021

nagisa commented Jul 29, 2021

joshtriplett commented Jul 29, 2021 via email

Amanieu commented Jul 30, 2021

RalfJung commented Jul 30, 2021

joshtriplett commented Jul 30, 2021 via email

bors commented Aug 8, 2021

This comment has been minimized.

Amanieu commented Aug 11, 2021

bors commented Aug 13, 2021

nbdd0121 Aug 14, 2021

nagisa Aug 14, 2021

nagisa commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 15, 2021

Amanieu commented Aug 15, 2021

bors commented Aug 15, 2021

bors commented Aug 15, 2021

bors commented Aug 16, 2021

Deprecate llvm_asm! #87590

Deprecate llvm_asm! #87590

Conversation

Amanieu commented Jul 29, 2021

This comment has been minimized.

joshtriplett commented Jul 29, 2021

Amanieu commented Jul 29, 2021

This comment has been minimized.

nagisa Jul 29, 2021

Choose a reason for hiding this comment

RalfJung Jul 29, 2021

Choose a reason for hiding this comment

Amanieu Jul 29, 2021

Choose a reason for hiding this comment

nagisa Aug 1, 2021

Choose a reason for hiding this comment

nagisa commented Jul 29, 2021

joshtriplett commented Jul 29, 2021 via email

Amanieu commented Jul 30, 2021

RalfJung commented Jul 30, 2021

joshtriplett commented Jul 30, 2021 via email

bors commented Aug 8, 2021

This comment has been minimized.

Amanieu commented Aug 11, 2021

bors commented Aug 13, 2021

nbdd0121 Aug 14, 2021

Choose a reason for hiding this comment

nagisa Aug 14, 2021

Choose a reason for hiding this comment

nagisa commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 15, 2021

Amanieu commented Aug 15, 2021

bors commented Aug 15, 2021

bors commented Aug 15, 2021

bors commented Aug 16, 2021