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

Add avr_skip for __udivti3 & __umodti3 #466

Merged
merged 1 commit into from
May 24, 2022
Merged

Add avr_skip for __udivti3 & __umodti3 #466

merged 1 commit into from
May 24, 2022

Conversation

Patryk27
Copy link
Contributor

@Patryk27 Patryk27 commented May 24, 2022 •
edited
Loading

I've forgotten to guard those two before and they also don't work at the moment (as confirmed by simavr).

A bit unfortunately, libgcc doesn't provide those functions, making i128/u128 division/modulo non-working on AVR (addition, subtraction and multiplication all seem to be working, as confirmed by random testing, so there's that, at least; hopefully no one plans to divide 128 bit numbers on AVRs 😛).

@bjorn3
Copy link
Member

bjorn3 commented May 24, 2022

Even if libgcc doesn't provide these, compiler-builtins can still provide them without issue, right?

@Amanieu Amanieu merged commit 32c0477 into rust-lang:master May 24, 2022
@Patryk27
Copy link
Contributor Author

Ah, sorry, probably should've linked to the previous pull request:
#462

tl;dr division and multiplication on AVR use custom calling convention (see Exceptions to the Calling Convention on https://gcc.gnu.org/wiki/avr-gcc), so for compiler-builtins to support those operations, we'd have to use hand-written naked functions (which is doable, but time-consuming and for little benefit, considering the fact that probably almost nobody will use those particular operations on an 8-bit MCU 😅)

@Patryk27 Patryk27 deleted the avr branch May 24, 2022 17:59
@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
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
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.
3 participants
@Patryk27 @bjorn3 @Amanieu