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

avr: Skip No More! #791

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Patryk27
Copy link
Contributor

@Patryk27 Patryk27 commented Mar 16, 2025

This pull request gets rid of all of the #[avr_skip]s introduced in the past.

My heart uC skips, skips, skips, skips, skips, skips a beat an intrinsic

... Olly Murs, frustrated about having to pull someone's intrinsics when working on AVR firmwares (colorized).

Changes here are related to #711, but note that we continue not to provide __u?divmod[hq]i4 - it shouldn't be terribly difficult to implement those (I even have a work-in-progress branch in rustc that exposes the proper calling convention), it's just a different kind of change that will be done in another pull request.

In general, the changes boil down to:

  • removing overzealous #[avr_skip] on intrinsics that don't actually have special convention (such as __addsf3),
  • implementing proper support for the other intrinsics (such as __unordsf2 or __divmodsi4).

I've tested new code via avr-tester - in particular, there's a test that uses randomized math operations and another one that checks floating point math, and both pass using the implementations provided by compiler-builtins.

Related discussions:

src/macros.rs Outdated
Comment on lines 259 to 260
#[allow(unused_imports)]
use super::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are all the new use super::* imports required?

Copy link
Contributor Author

@Patryk27 Patryk27 Mar 18, 2025

Choose a reason for hiding this comment

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

So that the CmpResult is in scope (see the discussion above on intrinsics! generating a new module for each function); we could use fully qualified paths, but that makes the code less readable imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that makes more sense. I think I would prefer use crate::float::cmp::CmpResult here, or -> crate::float::cmp::CmpResult in float::cmp since a glob import in the macro sounds like a recipe for future confusion. It would also be fine to reexport CmpResult back up to the top level too so that gets shortened to crate::CmpResult.

@tgross35
Copy link
Contributor

Also did you mean for this PR to have draft in the title, but not actually be marked as a draft? :)

@Patryk27
Copy link
Contributor Author

Also did you mean for this PR to have draft in the title, but not actually be marked as a draft? :)

Hah, #justgitlabthings

@Patryk27 Patryk27 marked this pull request as draft March 18, 2025 06:47
@tgross35
Copy link
Contributor

Also,

(I even have a work-in-progress branch in rustc that exposes the proper calling convention)

This shouldn't be necessary if there are only a couple of intrinsics. Instead, #[naked] + naked_asm! can be used, and the calling convention is just done in the assembly. The intrinsic implementation can be done in asm (probably better for performance), or trampoline to an extern "C" function we define.

@Patryk27
Copy link
Contributor Author

Patryk27 commented Mar 18, 2025

Ok, it's ready!

I've tested the code on ATmega328P and ATtiny167 (just simavr, since I've recently relocated and my electrical stuff is still in boxes in some warehouse, waiting for final delivery 😭).

@Patryk27 Patryk27 marked this pull request as ready for review March 18, 2025 17:11
@Patryk27 Patryk27 changed the title Draft: avr: Skip No More! avr: Skip No More! Mar 18, 2025
@tgross35
Copy link
Contributor

Sorry about the conflicts, this will need a rebase since I moved some things around (git should be able to figure it out). Just that and #791 (comment), then I think this will be ready to go.

@Patryk27
Copy link
Contributor Author

Okie, ready; I also added a comment to the CmpResult alias that links to LLVM, so that it's clear where the types come from.

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.

2 participants