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

Floating-point casts (sometimes) don't work on AVR #108489

Closed
Patryk27 opened this issue Feb 26, 2023 · 3 comments · Fixed by rust-lang/compiler-builtins#527 or #112763
Closed

Floating-point casts (sometimes) don't work on AVR #108489

Patryk27 opened this issue Feb 26, 2023 · 3 comments · Fixed by rust-lang/compiler-builtins#527 or #112763
Labels
A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.)

Comments

@Patryk27
Copy link
Contributor

This code:

#[atmega_hal::entry]
fn main() -> ! {
    let dp = Peripherals::take().unwrap();
    let pins = pins!(dp);

    let mut uart = Usart0::<MHz16>::new(
        dp.USART0,
        pins.pd0,
        pins.pd1.into_output(),
        115200u32.into_baudrate(),
    );

    let value: f32 = core::hint::black_box(1.0);
    let a = value as i64;

    ufmt::uwriteln!(&mut uart, "A: {:?}", a.to_le_bytes());

    loop {
        //
    }
}

... prints (under simavr):

A: [1, 0, 0, 0, 0, 0, 0, 0]

... but moving the code into a loop:

loop {
    let value: f32 = core::hint::black_box(1.0);
    let a = value as i64;

    ufmt::uwriteln!(&mut uart, "A: {:?}", a.to_le_bytes());
}

... breaks something:

A: [0, 0, 0, 128, 0, 0, 0, 128]

I'll try to investigate what's going on, I just thought I'll create an issue in case someone else stumbles upon this one as well.
@Patryk27 Patryk27 added the C-bug Category: This is a bug. label Feb 26, 2023
@Patryk27
Copy link
Contributor Author

Heh, ABI mismatch strikes again 😄

In this case it's caused by an intrinsic called __gtsf2() - the one from compiler-builtins takes priority and it's got a bit different ABI than the expected intrinsic from libgcc; lemme do some more tests and we'll probably #[avr_skip] those in compiler-builtins.

@Patryk27
Copy link
Contributor Author

Yeah; as a hot-fix, dropping this to .cargo/config should do it:

build-std-features = ["compiler-builtins-mangled-names"]

@workingjubilee workingjubilee added O-AVR Target: AVR processors (ATtiny, ATmega, etc.) A-floating-point Area: Floating point numbers and arithmetic labels Feb 26, 2023
Patryk27 added a commit to Patryk27/compiler-builtins that referenced this issue 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 issue Jun 12, 2023
Merged
@Patryk27
Copy link
Contributor Author

Patryk27 commented Jun 12, 2023 •
edited
Loading

(note that we still need to bump compiler-builtins in rustc for the patch to apply here, so it's actually semi-resolved at the moment)

@Patryk27 Patryk27 mentioned this issue Jun 18, 2023
Merged
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 24, 2023
@compiler-errors
Rollup merge of rust-lang#112763 - Patryk27:bump-compiler-builtins, r…
ee8b035 
…=Amanieu

Bump compiler_builtins

Actually closes rust-lang#108489.

Note that the example code given [in compiler_builtins](rust-lang/compiler-builtins#527) doesn't compile on current rustc since we're still waiting for https://reviews.llvm.org/D153197 (aka `LLVM ERROR: Expected a constant shift amount!`), but it's a step forward anyway.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Aug 24, 2023
@compiler-errors
Rollup merge of #112763 - Patryk27:bump-compiler-builtins, r=Amanieu
8a825b6 
Bump compiler_builtins

Actually closes rust-lang/rust#108489.

Note that the example code given [in compiler_builtins](rust-lang/compiler-builtins#527) doesn't compile on current rustc since we're still waiting for https://reviews.llvm.org/D153197 (aka `LLVM ERROR: Expected a constant shift amount!`), but it's a step forward anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Add #[avr_skip] for floats Patryk27/compiler-builtins
Bump compiler_builtins Patryk27/rust
2 participants
@Patryk27 @workingjubilee