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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix division on AVRs #462

Merged
merged 1 commit into from
May 24, 2022
Merged

Fix division on AVRs #462

merged 1 commit into from
May 24, 2022

Conversation

Patryk27
Copy link
Contributor

For division and modulo, AVR uses a custom calling convention that does
not match compiler_builtins' expectations, leading to non-working code¹.

Ideally we'd just use hand-written naked functions (as, afair, ARM
does), but that's a lot of code to port², so hopefully we'll be able to
do it gradually later.

For the time being, I'd suggest not compiling problematic functions for
AVR target - this causes avr-gcc (which is a mandatory part of Rust+AVR
toolchain anyway) to link hand-written assembly from libgcc, which is
confirmed to work.

I've tested the code locally on simavr and the patch seems to be working
correctly :-)

¹ rust-lang/rust#82242,
rust-lang/rust#83281
² https://github.com/gcc-mirror/gcc/blob/31048012db98f5ec9c2ba537bfd850374bdd771f/libgcc/config/avr/lib1funcs.S

Closes rust-lang/rust#82242
Closes rust-lang/rust#83281

@Patryk27
Fix division on AVRs
3f715fd 
For division and modulo, AVR uses a custom calling convention that does
not match compiler_builtins' expectations, leading to non-working code¹.

Ideally we'd just use hand-written naked functions (as, afair, ARM
does), but that's a lot of code to port², so hopefully we'll be able to
do it gradually later.

For the time being, I'd suggest not compiling problematic functions for
AVR target - this causes avr-gcc (which is a mandatory part of Rust+AVR
toolchain anyway) to link hand-written assembly from libgcc, which is
confirmed to work.

I've tested the code locally on simavr and the patch seems to be working
correctly :-)

¹ rust-lang/rust#82242,
  rust-lang/rust#83281
² https://github.com/gcc-mirror/gcc/blob/31048012db98f5ec9c2ba537bfd850374bdd771f/libgcc/config/avr/lib1funcs.S

Closes rust-lang/rust#82242
Closes rust-lang/rust#83281
@Amanieu Amanieu merged commit 5c2e8b0 into rust-lang:master May 24, 2022
@Patryk27 Patryk27 deleted the avr branch May 24, 2022 15:34
@Patryk27 Patryk27 mentioned this pull request May 24, 2022
Merged
@timwalls
Copy link

Hmm. I thought this change would mean I wouldn't need the compiler-builtins-mangled-names workaround any more, but without it the latest nightly (rustc 1.63.0-nightly (4cbaac699 2022-05-25)) still produces broken code.

Am I misinterpreting what this fix achieves, or has it not made it into nightly yet (exactly when things actually get merged to nightly is somewhat opaque to me :-))?

@Patryk27
Copy link
Contributor Author

has it not made it into nightly yet

Yes, this change hasn't made it into nightly yet -- hopefully in a few days! 🙂

@Amanieu
Copy link
Member

Amanieu commented May 26, 2022

I just published a new version of compiler-builtins. It now needs a PR in rust-lang/rust to update the dependency to the latest version.

@Patryk27
Copy link
Contributor Author

Oh neat, I'll prepare the PR in rust-lang, then

@timwalls
Copy link

has it not made it into nightly yet

Yes, this change hasn't made it into nightly yet -- hopefully in a few days! 🙂

Aha! Well, then be assured it's eagerly awaited and greatly appreciated, thank you. I shall go back to being patient :).

@Patryk27 Patryk27 mentioned this pull request Jun 14, 2022
Merged
Rahix pushed a commit to Rahix/avr-hal-template that referenced this pull request Jun 16, 2022
@Patryk27 @Rahix
Update toolchain, get rid of the std-features hack
dd810ff 
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242
Rahix added a commit to Rahix/avr-hal that referenced this pull request Jun 16, 2022
@Rahix
Update toolchain, get rid of the std-features hack
497442d 
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242

Suggested-by: @Patryk27
Link: Rahix/avr-hal-template#8
@Rahix Rahix mentioned this pull request Jun 16, 2022
Merged
Rahix added a commit to Rahix/avr-hal that referenced this pull request Jun 16, 2022
@Rahix
Update toolchain, get rid of the std-features hack
adc519f 
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242

Suggested-by: @Patryk27
Link: Rahix/avr-hal-template#8
Rahix added a commit to Rahix/avr-hal that referenced this pull request Jun 16, 2022
@Rahix
Update toolchain, get rid of the std-features hack
4f8ab25 
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242

Suggested-by: @Patryk27
Link: Rahix/avr-hal-template#8
Patryk27 added a commit to Patryk27/compiler-builtins that referenced this pull request Dec 25, 2022
@Patryk27
fix: Add #[avr_skip] for bit shifts
c9f5280 
This commit follows the same logic as:

- rust-lang#462
- rust-lang#466

I've tested the changes by preparing a simple program:

```rust
fn calc() -> ... {
    let x = hint::black_box(4u...); // 4u8, 4u16, 4u32, 4u64, 4u128 + signed
    let y = hint::black_box(1u32);

    // x >> y
    // x << y
}

fn main() -> ! {
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);
    let mut serial = arduino_hal::default_serial!(dp, pins, 57600);

    for b in calc().to_le_bytes() {
        _ = ufmt::uwrite!(&mut serial, "{} ", b);
    }

    _ = ufmt::uwriteln!(&mut serial, "");

    loop {
        //
    }
}
```

... switching types & operators in `calc()`, and observing the results;
what I ended up with was:

```
 u32 << u32 - ok
 u64 << u32 - ok
u128 << u32 - error (undefined reference to `__ashlti3')
 i32 >> u32 - ok
 i64 >> u32 - ok
i128 >> u32 - error (undefined reference to `__ashrti3')
 u32 >> u32 - ok
 u64 >> u32 - ok
u128 >> u32 - error (undefined reference to `__lshrti3')

(where "ok" = compiles and returns correct results)
```

As with multiplication and division, so do in here 128-bit operations
not work, because avr-gcc's standard library doesn't provide them (at
the same time, requiring that specific calling convention, making it
pretty difficult for compiler-builtins to jump in).

I think 128-bit operations non-working on an 8-bit controller is an
acceptable trade-off - :innocent: - and so the entire fix in here is
just about skipping those functions.
@Patryk27 Patryk27 mentioned this pull request Dec 25, 2022
Merged
@aykevl
Copy link

aykevl commented Dec 30, 2022

See: https://reviews.llvm.org/D138166

Patryk27 added a commit to Patryk27/compiler-builtins that referenced this pull request Jun 12, 2023
@Patryk27
fix: Add #[avr_skip] for floats
ec3b202 
Same story as always, i.e. ABI mismatch:

- rust-lang#462
- rust-lang#466
- rust-lang#513

I've made sure the changes work by rendering a Mandelbrot fractal:

```rust
#[arduino_hal::entry]
fn main() -> ! {
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);
    let mut serial = arduino_hal::default_serial!(dp, pins, 57600);

    mandelbrot(&mut serial, 60, 40, -2.05, -1.12, 0.47, 1.12, 100);

    loop {
        //
    }
}

fn mandelbrot<T>(
    output: &mut T,
    viewport_width: i64,
    viewport_height: i64,
    x1: f32,
    y1: f32,
    x2: f32,
    y2: f32,
    max_iterations: i64,
) where
    T: uWrite,
{
    for viewport_y in 0..viewport_height {
        let y0 = y1 + (y2 - y1) * ((viewport_y as f32) / (viewport_height as f32));

        for viewport_x in 0..viewport_width {
            let x0 = x1 + (x2 - x1) * ((viewport_x as f32) / (viewport_width as f32));

            let mut x = 0.0;
            let mut y = 0.0;
            let mut iterations = max_iterations;

            while x * x + y * y <= 4.0 && iterations > 0 {
                let xtemp = x * x - y * y + x0;
                y = 2.0 * x * y + y0;
                x = xtemp;
                iterations -= 1;
            }

            let ch = "#%=-:,. "
                .chars()
                .nth((8.0 * ((iterations as f32) / (max_iterations as f32))) as _)
                .unwrap();

            _ = ufmt::uwrite!(output, "{}", ch);
        }

        _ = ufmt::uwriteln!(output, "");
    }
}
```

... where without avr_skips, the code printed an image full of only `#`.

Note that because libgcc doesn't provide implementations for f64, using
those (e.g. swapping f32 to f64 in the code above) will cause linking to
fail:

```
undefined reference to `__divdf3'
undefined reference to `__muldf3'
undefined reference to `__gedf2'
undefined reference to `__fixunsdfsi'
undefined reference to `__gtdf2'
```

Ideally compiler-builtins could jump right in and provide those, but f64
also require a special calling convention which hasn't been yet exposed
through LLVM.

Note that because using 64-bit floats on an 8-bit target is a pretty
niche thing to do, and because f64 floats don't work correctly anyway at
the moment (due to this ABI mismatch), we're not actually breaking
anything by skipping those functions, since any code that currently uses
f64 on AVR works by accident.

Closes rust-lang/rust#108489.
@Patryk27 Patryk27 mentioned this pull request Jun 12, 2023
Merged
@Patryk27 Patryk27 mentioned this pull request Nov 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AVR: u32 and u64 behaves like u16 and u32 [AVR] integer division incorrectly yields result value 1
4 participants
@Patryk27 @timwalls @Amanieu @aykevl