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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

arch/cortex-m: change a few llvm_asm!s to asm!s #2092

Merged
merged 2 commits into from Sep 7, 2020
Merged

arch/cortex-m: change a few llvm_asm!s to asm!s #2092

merged 2 commits into from Sep 7, 2020

Conversation

JOE1994
Copy link
Contributor

@JOE1994 JOE1994 commented Sep 2, 2020

Pull Request Overview

This PR changes 3 invocations of llvm_asm! to asm!.
This PR intends to make a small step towards the direction of #1923 .
I made the changes after reading through the inline-asm RFC .

Brief overview of changes made
  1. llvm_asm!("nop" :::: "volatile"); => asm!("nop");
  2. llvm_asm!("wfi" :::: "volatile"); => asm!("wfi");
  • The only notable change in the first two cases is that the volatile keyword is gone. In the new asm! macro, volatile is assumed by default and there is an option called pure which can be used to get rid of the volatile semantics.
  1. llvm_asm!("mrs r0, ipsr": "={r0}"(interrupt_number) : :"r0" :);
    => asm!("mrs r0, ipsr", out("r0") interrupt_number);

TODO or Help Wanted

Although the changes made in this PR are quite simple, I'm not sure how to properly test the changes.
I'd appreciate any advice on testing strategy for this PR 馃樅

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@ppannuto
Copy link
Member

ppannuto commented Sep 2, 2020

Following the linked zulip post leads to these docs: https://github.com/rust-lang/rfcs/blob/master/text/2873-inline-asm.md#options-1

For the nop, I think we can add nomem, preserves_flags, and nostack.
For the wfi, I believe that we can/should also specify nomem [We might be able to add preserves_flags here too, but probably not nostack since the interrupt can/will push to the stack and that might muck with things depending what games the compiler is playing].
For the mrs, I think we can add nomem, preserves_flags, and nostack.

Regarding testing, I'm not sure there's so much that can be done beyond validating that basic functionality works (i.e. user process with interrupt, like buttons should catch the wfi and mrs; then any app that panics will exercise nop). The additional flags and options come into play when the compiler starts trying to optimize things, which isn't really easy to deterministically test around.

@JOE1994
Copy link
Contributor Author

JOE1994 commented Sep 4, 2020

Thank you for your feedback! I pushed the suggested updates :)

ppannuto
ppannuto approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

(Assuming tested on HW) looks good to me!

@JOE1994
Copy link
Contributor Author

JOE1994 commented Sep 5, 2020

I tested the current fork with 4 examples from libtock-rs: panic, blink, button_leds, button_subscribe.
At least these examples had no issues while running on the modified kernel :)

@ppannuto
Copy link
Member

ppannuto commented Sep 7, 2020

bors r+

Sounds good to me, thanks!!

@bors
Copy link
Contributor

bors bot commented Sep 7, 2020

Build succeeded:

@bors bors bot merged commit 602d0fb into tock:master Sep 7, 2020
@JOE1994 JOE1994 deleted the asm_cherrypick branch September 11, 2020 22:30
@JOE1994 JOE1994 mentioned this pull request Nov 30, 2020
Merged
2 tasks
bors bot added a commit that referenced this pull request Dec 7, 2020
@bors @JOE1994
Merge #2225
c8b1146 
2225: arch/cortex-m: change more llvm_asm! to asm! r=bradjc a=JOE1994

### Pull Request Overview

This PR changes 7 invocations of `llvm_asm!` to `asm!`.
Like #2092, this PR intends to make progress towards finishing #1923 .
I made the changes after reading through the [inline-asm RFC](https://github.com/Amanieu/rfcs/blob/inline-asm/text/0000-inline-asm.md#rules) .

#### A couple of notes on the changes
*  The first two `llvm_asm!` calls with `cc` clobbers are translated to `asm!` calls that don't explicitly state `cc` clobbers.
    In `asm!`, `cc` clobbers are assumed by default. (Option `preserve_flags` can be used when `cc` clobbers don't happen)
    * reference: [Rust Zulip chat stream 'project-inline-asm'](https://rust-lang.zulipchat.com/#narrow/stream/216763-project-inline-asm/topic/Moving.20forward.20with.20the.20RFC/near/185306638)
* `volatile` is assumed by default in `asm!`, so explicit `volatile` keywords were removed.
   (Option `pure` can be used to get rid of the `volatile` semantics)
   * reference: [Link to Q&A on `volatile` from Rust Zulip chat](https://rust-lang.zulipchat.com/#narrow/stream/216763-project-inline-asm/topic/volatile.20question/near/208320361)
*  `asm!` disallows use of `bool` variables as outputs. I replaced the output `bool` variables with `u32` variables.
```shell
   error: cannot use value of type `bool` for inline assembly
   --> arch/cortex-m/src/lib.rs:409:19
    |
409 |         out("r1") kernel_stack,
    |                   ^^^^^^^^^^^^
    |
    = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly
```

### Testing Strategy
I tested this PR by building the kernel and flashing to a `Hail` board with a blinky app.
Didn't see any erroneous behavior when pressing the reset button...

### TODO or Help Wanted
I somewhat naively replaced `bool` variables with `u32` variables.
Would it be better to replace the `bool` variables with `usize` instead?

### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.

### Thank you for reviewing this PR :crab: 

Co-authored-by: JOE1994 <joseph942010@gmail.com>
@duskmoon314 duskmoon314 mentioned this pull request Aug 11, 2021
Merged
bors bot added a commit to rust-embedded/riscv that referenced this pull request Aug 12, 2021
@bors @duskmoon314
Merge #86
5f386ef 
86: feat: Use new asm! instead of llvm_asm! r=Disasm a=duskmoon314

The new `asm` macro has been merged into `nightly` for some time, while `llvm_asm` will be gradually deprecated and will not enter `stable`. This PR replaces `llvm_asm` with `asm`, but there are still some minor issues to discuss.

My reference are:
- [asm feature](https://doc.rust-lang.org/unstable-book/library-features/asm.html)
- [arch/cortex-m: change a few llvm_asm!s to asm!s](tock/tock#2092)
- [Convert all uses of llvm_asm! to asm!](rust-lang/stdarch#1052)

---

In the new `asm` feature, there are several options can set to optimize codes. [options](https://doc.rust-lang.org/unstable-book/library-features/asm.html#options-1)

However, I'm not quite sure which options to set. (Sort of newbie to asm in rust)

Co-authored-by: Campbell He <hkp18@mails.tsinghua.edu.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppannuto ppannuto ppannuto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JOE1994 @ppannuto