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

Add llvm_asm! and deprecate asm! #2843

Merged
merged 4 commits into from Mar 20, 2020
Merged

Add llvm_asm! and deprecate asm! #2843

merged 4 commits into from Mar 20, 2020

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Jan 4, 2020

This is the first RFC from the inline asm project group!

Rendered

@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the RFC. A-ASM Proposals related to embedding assemly into Rust. labels Jan 6, 2020
@bstrie
Copy link
Contributor

bstrie commented Jan 7, 2020

I've been advocating for this rename for years, but I think this RFC is leaving too much unspoken in its current form. In particular:

  1. Are we certain that a macro is ideal ultimate form for stable inline assembly, as opposed to e.g. a keyword-prefixed block along the lines of async or extern? If we eventually chose not to use a macro here, then this rename may be unnecessary.

  2. Even assuming that a macro is the best way to represent inline assembly in Rust code, is asm! the name that we want it to have? If not, then a rename would be unnecessary.

Furthermore, passages from the text such as the following:

No silent miscompilations are expected since the operand separator will be changed from : to ,, which will guarantee that any existing asm! invocations will fail with a syntax error with the new asm! macro.

...suggest that there already exists a tentative design document for a stable inline assembly proposal. I would expect the RFC to contain a link to that document for further reading.

@Amanieu
Copy link
Member Author

Amanieu commented Jan 7, 2020

Are we certain that a macro is ideal ultimate form for stable inline assembly, as opposed to e.g. a keyword-prefixed block along the lines of async or extern? If we eventually chose not to use a macro here, then this rename may be unnecessary.

Unfortunately we can't use an asm keyword since it isn't reserved, at least until the next edition.

Even assuming that a macro is the best way to represent inline assembly in Rust code, is asm! the name that we want it to have? If not, then a rename would be unnecessary.

That is the current plan. Here is the current RFC draft from the inline asm project group: https://github.com/rust-lang/project-inline-asm/blob/master/rfcs/0000-inline-asm.md

@Diggsey
Copy link
Contributor

Diggsey commented Jan 7, 2020

Unfortunately we can't use an asm keyword since it isn't reserved, at least until the next edition.

I'd question whether we'd even want new syntax (that must then be supported by all rust parsers) for assembly when its use is likely to be contained to a small proportion of crates. Being a macro is also more consistent with the formatting macros the new design tries to imitate.

@bstrie
Copy link
Contributor

bstrie commented Jan 7, 2020

One more question that I would like to see the RFC address: is it the recommendation of the ASM working group that the hypothetical llvm_asm! macro should eventually be stabilized, or will it, like the current asm! macro, be considered essentially permanently unstable? Because if it's the latter, then I think there will be a sizeable contingent calling for the outright removal of the old macro if ever there exists a stable and portable alternative. And if it's the former then I think that raises questions about whether that suggests deficiencies in the stable inline assembler support (though that hypothetical discussion would be best left for another RFC).

EDIT: I didn't see the above link to the asm! proto-RFC until after posting this comment, which does note that llvm_asm! is not intended to ever be stabilized; I would recommend mentioning this in the RFC here.

@fintelia
Copy link

fintelia commented Jan 9, 2020

No silent miscompilations are expected since the operand separator will be changed from : to ,, which will guarantee that any existing asm! invocations will fail with a syntax error with the new asm! macro.

What about invocations that don't use the operand separator? Currently asm!("nop") is completely valid, and might not be a syntax error in the new design. (With this case there is a high probability that the behavior would be unchanged, but I think it is too early to know whether this would always be true)

@Amanieu
Copy link
Member Author

Amanieu commented Jan 9, 2020

For the specific case of asm with no operands, the behavior is almost identical. The main difference is that the new asm! will have volatile and memory clobbers on by default, but this is fine since it is strictly more conservative.

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo nitpick this LGTM.

Deprecate the existing `asm!` macro and provide an identical one called
`llvm_asm!`. The feature gate is also renamed from `asm` to `llvm_asm`.

Unlike `asm!`, `llvm_asm!` is not intended to ever become stable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds a bit too strong, e.g, if we ever add a #[cfg(codegen_backed = "...")] macro to detect the code generation backend, then using llvm_asm! behind a #[cfg(codegen_backend = "llvm")] might make sense.

Suggested change
Unlike `asm!`, `llvm_asm!` is not intended to ever become stable.
Unlike `asm!`, `llvm_asm!` is currently perma unstable.

Also, if the feature is intended to be perma-unstable, at least for the time being, it might make sense to name the feature gate: feature(rustc_llvm_asm).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last that I checked (which was a while ago, mind), there are more objections to stabilizing the existing asm! macro than just its coupling to LLVM. Furthermore, if the proto-RFC for stable ASM were accepted and implemented it would be sufficiently different from the existing ASM support (both syntactically and semantically) that I feel people would be reluctant to stabilize the old version, if only for the sake of consistency.

This isn't to say anything about whether a stable llvm_asm! might be desirable; only that if it is desirable, it would likely not be served by merely stabilizing the existing ASM macro. I would expect such a discussion to be the purview of a totally separate RFC, although if the possibility isn't completely remote then that could suggest leaving the name llvm_asm! free for future use. I do like the idea of prepending "rustc" to things which are not intended to ever be stabilized... but really the "rustc" prefix should only be used for perma-unstable compiler implementation details, and unless rustc is using asm! (and I don't see why it would), I'd much prefer planning to remove the original asm! sometime after a replacement has been stabilized.

Copy link

@comex comex Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My position is that the new asm! should be designed to provide substantially all of the functionality of the old one – and eventually more, such as an equivalent of C asm goto, which is supported by LLVM but not currently exposed in Rust.* After all... well, I don't want to downplay the complexity of implementing inline assembly in the first place. But since we are planning to do that, the question is how much additional complexity would be required to get from "basic support" to "full parity". As I see it, most of that complexity comes from two sources:

  1. The large amount of functionality supported by the assembler itself (e.g. directives, preprocessor)... but we're aiming to target existing assemblers, not create our own.

  2. The smorgasbord of arch-dependent constraint codes supported by GCC... but LLVM only supports a small fraction of them anyway, and we only need to match LLVM's support.

So it should be feasible to reach pretty much full parity. At that point, llvm_asm! will be redundant, and I think it's fine to plan to remove it. That said, there would be no urgent need to remove it; it should be possible to have a long deprecation period, and there might even be a case for keeping it indefinitely for experimentation purposes... but that would have to be weighed against the compiler maintenance burden.

* For anyone who hasn't been following discussions, that doesn't mean adding goto to Rust; we just need some way to express the concept of jumping out of asm blocks, which would most likely resemble a match statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing we need to unblock progress on the new inliner assembler is to rename the asm! macro to llvm_asm!, although as exposed here it also makes sense to rename the feature gate.

While I mostly agree with what you said, we don't really need to make any commitments right now about the future of llvm_asm!, so we can just punt that discussion.

@JonSeverinsson
Copy link

Personally I find the proposed new name (llvm_asm!) very confusing, as to me that would signify the insertion of LLVM assembly language snippets, rather than insertion x86/whatever assembly language snippets to be passed to LLVM...

@bstrie
Copy link
Contributor

bstrie commented Jan 10, 2020

I don't think the choice of replacement name is too important, as it's unlikely that the renamed macro will ever see stability... but if we're going to descend into bikeshedding, then I'll suggest legacy_asm! as an alternative.

@Lokathor
Copy link
Contributor

The semantics of the temporary old-style macro would boil down to "whatever LLVM does", which is why it's llvm_asm!.

We could clearly put that in the docs for the macro, and hopefully no one is calling unsafe code without reading the docs of what they're calling.

@Ixrec
Copy link
Contributor

Ixrec commented Jan 11, 2020

I didn't even know "LLVM assembly language" was a (the?) correct term for it until just now, since everyone always calls it LLVM IR in practice. So IMO llvm_asm!() is still the best name overall. Though it'll likely never be stabilized, so it's not too important either way.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 12, 2020

Personally I find the proposed new name (llvm_asm!) very confusing, as to me that would signify the insertion of LLVM assembly language snippets, rather than insertion x86/whatever assembly language snippets to be passed to LLVM...

We could call it clang_asm! or c_asm! since that's closer to what the macro actually does - it takes syntax closer to clang's C asm(...) statements and maps it to LLVM-IR inline assembler expressions.

I personally don't find the name llvm_asm! confusing but don't mind either way since the name isn't that important for something that's currently not in the path towards stabilization.

@programmerjake
Copy link
Member

Personally I find the proposed new name (llvm_asm!) very confusing, as to me that would signify the insertion of LLVM assembly language snippets, rather than insertion x86/whatever assembly language snippets to be passed to LLVM...

We could call it clang_asm! or c_asm! since that's closer to what the macro actually does - it takes syntax closer to clang's C asm(...) statements and maps it to LLVM-IR inline assembler expressions.

I personally don't find the name llvm_asm! confusing but don't mind either way since the name isn't that important for something that's currently not in the path towards stabilization.

How about llvm_style_asm! for LLVM-style assembly language? That should distinguish it from LLVM IR, since that would be called LLVM IR or LLVM assembly language without "style". I wouldn't worry about the extra identifier length, since using inline-assembly is rare enough anyway, and since extra clarity is always useful around inline assembly anyway.

@nagisa
Copy link
Member

nagisa commented Jan 12, 2020

We could call it clang_asm! or c_asm! since that's closer to what the macro actually does - it takes syntax closer to clang's C asm(...) statements and maps it to LLVM-IR inline assembler expressions.

It is closer, but would be even more confusing than just llvm_asm!, because what people would expect it to be compatible with gcc inline assembly. LLVM's inline assembly is not compatible with gcc’s and clangs and this assumption has resulted in multiple issues being filled against our current asm! implementation.

EDIT: presuming we don't want to reimplement the same gcc -> llvm translation layer that clang implements.

@Amanieu
Copy link
Member Author

Amanieu commented Jan 21, 2020

WIP PR: rust-lang/rust#68404

@nikomatsakis

This comment has been minimized.

@rfcbot

This comment has been minimized.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Feb 27, 2020
@nikomatsakis

This comment has been minimized.

@rfcbot

This comment has been minimized.

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Feb 27, 2020
@rfcbot rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Mar 8, 2020
@Amanieu
Copy link
Member Author

Amanieu commented Mar 16, 2020

FCP finished a week ago, could someone with the appropriate permissions merge the RFC?

@Amanieu Amanieu merged commit f26234b into rust-lang:master Mar 20, 2020
@Amanieu Amanieu deleted the llvm-asm branch March 20, 2020 07:14
@Amanieu
Copy link
Member Author

Amanieu commented Mar 20, 2020

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/rust#70173

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2020
Rename asm! to llvm_asm!

As per rust-lang/rfcs#2843, this PR renames `asm!` to `llvm_asm!`. It also renames the compiler's internal `InlineAsm` data structures to `LlvmInlineAsm` in preparation for the new `asm!` functionality specified in rust-lang/rfcs#2850.

This PR doesn't actually deprecate `asm!` yet, it just makes it redirect to `llvm_asm!`. This is necessary because we first need to update the submodules (in particular stdarch) to use `llvm_asm!`.
bors bot added a commit to Amanieu/parking_lot that referenced this pull request Apr 9, 2020
223: Use llvm_asm! instead of asm! r=Amanieu a=Amanieu

`asm!` will be deprecated soon in preparation for the new `asm!` macro, so switch to using `llvm_asm!` instead.

cc rust-lang/rfcs#2843

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
bors bot added a commit to Amanieu/parking_lot that referenced this pull request Apr 9, 2020
223: Use llvm_asm! instead of asm! r=Amanieu a=Amanieu

`asm!` will be deprecated soon in preparation for the new `asm!` macro, so switch to using `llvm_asm!` instead.

cc rust-lang/rfcs#2843

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
bors bot added a commit to Amanieu/parking_lot that referenced this pull request Apr 9, 2020
223: Use llvm_asm! instead of asm! r=Amanieu a=Amanieu

`asm!` will be deprecated soon in preparation for the new `asm!` macro, so switch to using `llvm_asm!` instead.

cc rust-lang/rfcs#2843

Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
Centril added a commit to Centril/rust that referenced this pull request Apr 11, 2020
…acrum

Deprecate the asm! macro in favor of llvm_asm!

Since we will be changing the syntax of `asm!` soon, deprecate it and encourage people to use `llvm_asm!` instead (which preserves the old syntax). This will avoid breakage when `asm!` is changed.

RFC: rust-lang/rfcs#2843
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2020
Deprecate the asm! macro in favor of llvm_asm!

Since we will be changing the syntax of `asm!` soon, deprecate it and encourage people to use `llvm_asm!` instead (which preserves the old syntax). This will avoid breakage when `asm!` is changed.

RFC: rust-lang/rfcs#2843
tjhu added a commit to mars-research/redleaf that referenced this pull request May 29, 2020
bors bot added a commit to rust-embedded/svd2rust that referenced this pull request Jun 2, 2020
446: update msp430 test deps r=therealprof a=burrbull

rust-lang/rfcs#2843

r? @therealprof 

Co-authored-by: Andrey Zgarbul <zgarbul.andrey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ASM Proposals related to embedding assemly into Rust. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet