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] Shift left << doesn't work correctly on >= 32-bit integers on a 16-bit platform #82380

Closed
jacobmischka opened this issue Feb 21, 2021 · 5 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jacobmischka
Copy link

jacobmischka commented Feb 21, 2021

Shift left seems to not behave correctly, but doing the same resulting operation using multiplication does work as a workaround. This occurs both when using the << operator and when using overflowing_shl. This was tested on an Arduino Nano (actually an Elegoo knockoff with a CH340, link).

I tried this code:

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;
use ufmt::uwriteln;

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

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for i in 0..64 {
        uwriteln!(&mut serial, "1 << {}\r", i).void_unwrap();

        let shl: u64 = 1u64 << i;
        uwriteln!(&mut serial, "Shift: {:?}\r", shl.to_le_bytes()).void_unwrap();

        let mul: u64 = 1u64 * 2u64.pow(i as u32);
        uwriteln!(&mut serial, "Mul and pow: {:?}\r\n", mul.to_le_bytes()).void_unwrap();
    }

    loop {}
}
Output:
1 << 0
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [1, 0, 0, 0, 0, 0, 0, 0]

1 << 1
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [2, 0, 0, 0, 0, 0, 0, 0]

1 << 2
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [4, 0, 0, 0, 0, 0, 0, 0]

1 << 3
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [8, 0, 0, 0, 0, 0, 0, 0]

1 << 4
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [16, 0, 0, 0, 0, 0, 0, 0]

1 << 5
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [32, 0, 0, 0, 0, 0, 0, 0]

1 << 6
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [64, 0, 0, 0, 0, 0, 0, 0]

1 << 7
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [128, 0, 0, 0, 0, 0, 0, 0]

1 << 8
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 1, 0, 0, 0, 0, 0, 0]

1 << 9
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 2, 0, 0, 0, 0, 0, 0]

1 << 10
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 4, 0, 0, 0, 0, 0, 0]

1 << 11
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 8, 0, 0, 0, 0, 0, 0]

1 << 12
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 16, 0, 0, 0, 0, 0, 0]

1 << 13
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 32, 0, 0, 0, 0, 0, 0]

1 << 14
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 64, 0, 0, 0, 0, 0, 0]

...

1 << 51
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 8, 0]

1 << 52
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 16, 0]

1 << 53
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 32, 0]

1 << 54
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 64, 0]

1 << 55
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 128, 0]

1 << 56
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 1]

1 << 57
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 2]

1 << 58
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 4]

1 << 59
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 8]

1 << 60
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 16]

1 << 61
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 32]

1 << 62
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 64]

1 << 63
Shift: [1, 0, 1, 0, 1, 0, 0, 0]
Mul and pow: [0, 0, 0, 0, 0, 0, 0, 128]
Same code, except `u32` instead of `u64`
1 << 0
Shift: [0, 128, 0, 0]
Mul and pow: [1, 0, 0, 0]

1 << 1
Shift: [0, 128, 0, 0]
Mul and pow: [2, 0, 0, 0]

1 << 2
Shift: [0, 128, 0, 0]
Mul and pow: [4, 0, 0, 0]

1 << 3
Shift: [0, 128, 0, 0]
Mul and pow: [8, 0, 0, 0]

1 << 4
Shift: [0, 128, 0, 0]
Mul and pow: [16, 0, 0, 0]

1 << 5
Shift: [0, 128, 0, 0]
Mul and pow: [32, 0, 0, 0]

1 << 6
Shift: [0, 128, 0, 0]
Mul and pow: [64, 0, 0, 0]

1 << 7
Shift: [0, 128, 0, 0]
Mul and pow: [128, 0, 0, 0]

1 << 8
Shift: [0, 128, 0, 0]
Mul and pow: [0, 1, 0, 0]

1 << 9
Shift: [0, 128, 0, 0]
Mul and pow: [0, 2, 0, 0]

1 << 10
Shift: [0, 128, 0, 0]
Mul and pow: [0, 4, 0, 0]

1 << 11
Shift: [0, 128, 0, 0]
Mul and pow: [0, 8, 0, 0]

1 << 12
Shift: [0, 128, 0, 0]
Mul and pow: [0, 16, 0, 0]

1 << 13
Shift: [0, 128, 0, 0]
Mul and pow: [0, 32, 0, 0]

1 << 14
Shift: [0, 128, 0, 0]
Mul and pow: [0, 64, 0, 0]

1 << 15
Shift: [0, 128, 0, 0]
Mul and pow: [0, 128, 0, 0]

1 << 16
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 1, 0]

1 << 17
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 2, 0]

1 << 18
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 4, 0]

1 << 19
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 8, 0]

1 << 20
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 16, 0]

1 << 21
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 32, 0]

1 << 22
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 64, 0]

1 << 23
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 128, 0]

1 << 24
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 1]

1 << 25
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 2]

1 << 26
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 4]

1 << 27
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 8]

1 << 28
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 16]

1 << 29
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 32]

1 << 30
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 64]

1 << 31
Shift: [0, 128, 0, 0]
Mul and pow: [0, 0, 0, 128]

Meta

rustc --version --verbose:

rustc 1.51.0-nightly (c2de47a9a 2021-01-06)
binary: rustc
commit-hash: c2de47a9aa4c9812884f341f1852e9c9610f5f7a
commit-date: 2021-01-06
host: x86_64-unknown-linux-gnu
release: 1.51.0-nightly

This might be related to #82242. As in that issue, I am using this version of nightly because it's the latest that supports AVR, and it must be built using --release.

Full reproduction repository: https://github.com/jacobmischka/rust-avr-shl

@jacobmischka jacobmischka added the C-bug Category: This is a bug. label Feb 21, 2021
@jonas-schievink jonas-schievink added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) labels Feb 21, 2021
@jacobmischka
Copy link
Author

This probably is related to #82242, because shifting does work for u16:

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;
use ufmt::uwriteln;

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

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for i in 0u16..16u16 {
        uwriteln!(&mut serial, "1 << {}\r", i).void_unwrap();

        let shl: u16 = 1u16 << i;
        uwriteln!(&mut serial, "Shift: {:?}\r", shl.to_le_bytes()).void_unwrap();

        let mul: u16 = 1u16 * 2u16.pow(i as u32);
        uwriteln!(&mut serial, "Mul and pow: {:?}\r\n", mul.to_le_bytes()).void_unwrap();
    }

    loop {}
}

Output:

1 << 1
Shift: [2, 0]
Mul and pow: [2, 0]

1 << 2
Shift: [4, 0]
Mul and pow: [4, 0]

1 << 3
Shift: [8, 0]
Mul and pow: [8, 0]

1 << 4
Shift: [16, 0]
Mul and pow: [16, 0]

1 << 5
Shift: [32, 0]
Mul and pow: [32, 0]

1 << 6
Shift: [64, 0]
Mul and pow: [64, 0]

1 << 7
Shift: [128, 0]
Mul and pow: [128, 0]

1 << 8
Shift: [0, 1]
Mul and pow: [0, 1]

1 << 9
Shift: [0, 2]
Mul and pow: [0, 2]

1 << 10
Shift: [0, 4]
Mul and pow: [0, 4]

1 << 11
Shift: [0, 8]
Mul and pow: [0, 8]

1 << 12
Shift: [0, 16]
Mul and pow: [0, 16]

1 << 13
Shift: [0, 32]
Mul and pow: [0, 32]

1 << 14
Shift: [0, 64]
Mul and pow: [0, 64]

1 << 15
Shift: [0, 128]
Mul and pow: [0, 128]

@jacobmischka jacobmischka changed the title [AVR] Shift left << seems to be broken [AVR] Shift left << doesn't work correctly on >= 32-bit integers on a 16-bit platform Feb 27, 2021
@Patryk27
Copy link
Contributor

Seems to be the same root cause as #82242; the workaround helps 🙂

@gergoerdi
Copy link

I just bumped into this on nighty-2022-12-09, one and a half years after #82242 got closed. Indeed the workaround in #82242 (comment) solved my issue. However, with #82242 closed already, why is the workaround still needed?

Is it simply a matter of some u32 and u64 operations working now, but shifts need to be added separately?

@workingjubilee
Copy link
Member

There is indeed probably a need for checks and fixups in that general vein specific to shl.

@Patryk27
Copy link
Contributor

fwiw, this got fixed with #114048 🙂

@nikic nikic closed this as completed Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants