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 options to asm #12

Merged
merged 1 commit into from
Sep 11, 2022
Merged

Add options to asm #12

merged 1 commit into from
Sep 11, 2022

Conversation

taiki-e
Copy link
Contributor

@taiki-e taiki-e commented Aug 4, 2022

refs: https://doc.rust-lang.org/nightly/reference/inline-assembly.html#options

cc #11
cc @cr1901

@taiki-e taiki-e requested a review from a team as a code owner August 4, 2022 17:54
src/asm.rs Outdated
@@ -6,14 +6,15 @@ use crate::asm;
#[inline(always)]
pub fn nop() {
unsafe {
asm!("nop");
asm!("nop", options(nostack, preserves_flags));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The optimization we want to prevent here is the removal of asm (nop), right?
If so, perhaps adding nomem and a comment like "Do not use pure because prevent nop from being removed" would be preferable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm using nop, I'm probably using it for timing purposes. For timing between I/O reads/writes, which are inherently volatile (and thus won't be moved around the nop) there is no problem. But if I'm trying to time between doing an algorithm and an I/O read/write, I don't want the end of the algorithm to be moved after the nop.

So I think I want to hold off on adding nomem. The remaining flags look fine to me, I just want to test locally and see what happens w/ a test firmware :).

Copy link
Contributor Author

@taiki-e taiki-e Aug 5, 2022

Choose a reason for hiding this comment

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

But if I'm trying to time between doing an algorithm and an I/O read/write, I don't want the end of the algorithm to be moved after the nop.

Hmm, I wonder if it would be preferable to explicitly use barriers in this use case.

Also, cortex-m crate seems to use nomem in nop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be preferable to explicitly use barriers in this use case.

Inside the nop() fn implementation, or tell the user to add them manually when calling nop()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant the latter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Barrier looks like it does what nop does just without nomem. I think for something like nop we should follow what cortex-m does and use barrier for timing usecases. We should also document this distinction.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for something like nop we should follow what cortex-m does and use barrier for timing usecases.

Fair enough; if the cortex-m ppl change nop behavior later, we can follow suit.

We should also document this distinction.

@taiki-e Would you be willing to add this documentation w/ an example, and then I'll accept this? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@taiki-e Did you get the chance to update this PR w/ documentation and an example? I would like to cut a release.

If not, we can wait until v0.4.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated docs, but I'm not sure what is a good example here, so I didn't add examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, no need for example. I meant an example like "put barriers before and after the nop sled" if the exact number of nops between I/O access is important".

In any case, merging this, thank you!

- Use `nomem` when neither memory access nor reordering prevention is needed.
- Use `nostack` when not pushing data to the stack.
- Use `preserves_flags` when not modifying the status register.
  refs: https://doc.rust-lang.org/nightly/unstable-book/language-features/asm-experimental-arch.html#flags-covered-by-preserves_flags
- Add comments explaining why specific options are not used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants