fix: Add #[avr_skip] for bit shifts

[Patryk27]

This commit follows the same logic as:

• Fix division on AVRs #462
• Add avr_skip for __udivti3 & __umodti3 #466

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

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 - 😇 - and so the entire fix in here is just about skipping those functions.

Related to: rust-lang/rust#106135

[JohnTitor]

Sounds sensible to me.

[aykevl]

I wrote AVRShiftExpand so you can ask me about it :)
(And I'm about to replace it with something better soon)

Regarding the bug here, I'm pretty sure this line is not correct:

__ashldi3(a: u64, b: u32) -> u64 {

For AVR, b is u8 not u32.

[aykevl]

Somewhat unrelated, but is there any reason why Rust uses the Rust clone of compiler-rt instead of the original project for AVR? It would avoid a lot of duplicate effort: @benshi001 and me have been working on getting compiler-rt fully supported on AVR.

[Patryk27]

I might be not up to par in terminology -- what in this case would be the original project for AVR? 👀

[aykevl]

So compiler-builtins is essentially a Rust clone of compiler-rt (a LLVM project). I wonder whether it wouldn't be better to use compiler-rt (instead of compiler-builtins) for AVR?
The reasoning is:

1. AVR relies heavily on a libgcc/compiler-rt like library because it is such a limited architecture.
2. AVR is very different from other architectures with a much smaller machine word size. This has effects on the ABI and results in various kinds of bugs - as can be seen in this PR.
3. Many of these bugs have been fixed already in compiler-rt (written in C) - including some that I fixed myself. And more are to be fixed in follow-up patches.
4. IMHO it's a waste to duplicate this effort in compiler-builtins, especially considering the limited resources available for AVR development.

I guess compiler-butiltins is generally preferred in Rust because it is also written in Rust. My personal opinion is that it doesn't make much sense to rewrite this kind of thing in Rust because it's so low level and ABI heavy. But clearly at least some people disagree otherwise compiler-builtins wouldn't exist. For AVR in particular I wonder whether it wouldn't be better to use compiler-rt instead and focus on improving it for everybody instead of duplicating the effort in Rust.

For context: I maintain TinyGo and I've switched away from the GNU toolchain (libgcc, etc) entirely. Most parts of compiler-rt work fine in my testing - including float32 and float64 builtins (but with the notable exception of 32-bit multiplication/division functions that are not terribly hard to implement in Go/Rust/C/whatever). I think the Rust project could do the same to avoid bugs like the one that this PR fixes.

[Patryk27]

I see - yeah, I'd say that for AVR it would make more sense to go with LLVM's compiler-rt instead of compiler-builtins; on the top of my head I see no reason why that wouldn't work 🙂

[Amanieu]

Of the top of my head I know of several reasons why we are not using compiler-rt directly:

• On some targets (e.g. ARM) the float builtins use FP instructions in compiler-rt while compiler-builtins always uses soft-float. We prefer it this way so that floating-point types are fully usable on targets without floating-point (e.g. kernel code).
• Rust exposes 128-bit integers on all targets instead of just 64-bit targets like Clang/GCC. This means that the required compiler-rt builtin functions cannot be written in C, since C doesn't have __uint128_t on 32-bit targets.

With that said, we do have a build-time option to use implementations from compiler-rt for some builtin functions where compiler-rt provides optimized assembly implementations. The only downside is that this is not used when building with -Zbuild-std so it will be of little use for tier 3 targets such as AVR.

[aykevl]

On some targets (e.g. ARM) the float builtins use FP instructions in compiler-rt while compiler-builtins always uses soft-float. We prefer it this way so that floating-point types are fully usable on targets without floating-point (e.g. kernel code).

Me too. This seems to work fine for me though? It may be needed to use the generic files instead of the optimized assembly in some cases though, but something like __addsf3 for example is entirely soft-float.

Rust exposes 128-bit integers on all targets instead of just 64-bit targets like Clang/GCC. This means that the required compiler-rt builtin functions cannot be written in C, since C doesn't have __uint128_t on 32-bit targets.

This could nowadays be supported though _BitInt(128). But it isn't supported in compiler-rt yet and I'm not even sure the ABI has been defined yet.

Anyway, it's really just a suggestion. I'm not really working with Rust.