Compiler generates wrong code for VPORTA.output (port at 0x1)

[tomtor]

Somewhat related to https://github.com/Rahix/avr-device/issues/190 and https://github.com/rust-lang/rust/pull/141260 (@LuigiPiucco )

I discovered this in a normal avr-device program: https://github.com/tomtor/avr-modern-rust

and rewrote it to a minimal program to demonstrate the issue:

#![no_std]
#![no_main]

// const IO: u16 = 0x1; // this value generated bad code
const IO: u16 = 0x2; // this value generates good code

const VPORTA: *mut u8 = (IO) as *mut u8;

const LED: u8 = 0b1000_0000; // PA7


pub fn high_vp(b: u8) {
    unsafe {
        *VPORTA = *VPORTA | b;
    }
}

#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
    loop {}
}

// the body of the next function is nonsense and irrelevant, we just need some code which is not inlined
pub fn nil(a: u16) {
    let mut i: u16 = 0;
    while i < 100 {
        high_vp(LED);
        nil(a - 1);
        i += 1;
    }
}

#[avr_device::entry]
//#[cortex_m_rt::entry]
fn main() -> ! {
    let mut counter: u16 = 0;

    loop {
        counter += 1;

        // The following line produces incorrect code: !!!
        high_vp(LED);

        nil((counter & 0xf) + IO);
    }
}

this generates the following excellent code:

00000094 <_ZN15avr_modern_demo20__avr_device_rt_main17h009e886c0b40ab89E>:
  94:   0f 93           push    r16
  96:   1f 93           push    r17
  98:   80 e0           ldi     r24, 0x00       ; 0
  9a:   90 e0           ldi     r25, 0x00       ; 0
  9c:   17 9a           sbi     0x02, 7 ; 2
  9e:   01 96           adiw    r24, 0x01       ; 1
  a0:   8c 01           movw    r16, r24
  a2:   8f 70           andi    r24, 0x0F       ; 15
  a4:   90 70           andi    r25, 0x00       ; 0
  a6:   02 96           adiw    r24, 0x02       ; 2
  a8:   0e 94 2b 00     call    0x56    ; 0x56 <_ZN15avr_modern_demo3nil17h6637efea0a821bd7E>
  ac:   c8 01           movw    r24, r16
  ae:   f6 cf           rjmp    .-20            ; 0x9c <_ZN15avr_modern_demo20__avr_device_rt_main17h009e886c0b40ab89E+0x8>

however, if we use 0x1 instead of 0x2 we get:

00000096 <_ZN15avr_modern_demo20__avr_device_rt_main17h009e886c0b40ab89E>:
  96:   0f 93           push    r16
  98:   1f 93           push    r17
  9a:   a0 e0           ldi     r26, 0x00       ; 0
  9c:   b0 e0           ldi     r27, 0x00       ; 0
  9e:   8d 91           ld      r24, X+
  a0:   80 68           ori     r24, 0x80       ; 128
  a2:   8d 01           movw    r16, r26
  a4:   af 70           andi    r26, 0x0F       ; 15
  a6:   b0 70           andi    r27, 0x00       ; 0
  a8:   8d 93           st      X+, r24
  aa:   cd 01           movw    r24, r26
  ac:   0e 94 2b 00     call    0x56    ; 0x56 <_ZN15avr_modern_demo3nil17h6637efea0a821bd7E>
  b0:   d8 01           movw    r26, r16
  b2:   f5 cf           rjmp    .-22            ; 0x9e <_ZN15avr_modern_demo20__avr_device_rt_main17h009e886c0b40ab89E+0x8>

The varying argument to nil() is used to store the new contents at 0x1 which is only correct for the first iteration of the loop!

[tomtor]

Just tested witch clang:

clang -O2 -target avr-none -mmcu=attiny402 -S bug.c

and bug.c:

#define PA      0x1 // Bad                                                                                                          
//#define PA    0x2 // Good

char *const PORT = (char *const) PA;

extern void nil(short);

void bug()
{
  short s= 0;
 
  while (1) {
    s++;
    *PORT = *PORT | 0x80;
    nil((s & 0xF) + 1);
  }
}

results in:

bug:                                    ; @bug
; %bb.0:                                ; %entry
        push    r16
        push    r17
        ldi     r26, 0
        ldi     r27, 0
.LBB0_1:                                ; %while.cond
                                        ; =>This Inner Loop Header: Depth=1
        ld      r24, X+
        ori     r24, -128
        movw    r16, r26
        andi    r26, 15
        andi    r27, 0
        st      X+, r24
        movw    r24, r26
        call    nil
        movw    r26, r16
        rjmp    .LBB0_1

So its is a generic LLVM issue.

[tomtor]

created an LLVM issue: https://github.com/llvm/llvm-project/issues/143247

[LuigiPiucco]

This seems unrelated to the issues with address 0x0, those cause a call to panic to be generated, and they should really only affect 0x0 in Rust. It appears to be more of a missed optimization. And like you noticed, it is present in clang, so handling it in LLVM is likely to be more successful. It's an important issue, but discussion should probably continue over at LLVM.

[tomtor]

This seems unrelated to the issues with address 0x0, those cause a call to panic to be generated, and they should really only affect 0x0 in Rust. It appears to be more of a missed optimization. And like you noticed, it is present in clang, so handling it in LLVM is likely to be more successful. It's an important issue, but discussion should probably continue over at LLVM.

I agree. We can leave this issue open for now as a warning for AVR users?

[LuigiPiucco]

I'd say yes, but that's up to @Rahix. I don't have permission to close issues anyway.

[Rahix]

Cc @Patryk27

[Rahix]

Moved this to avr-hal where we do track a bunch of known miscompilations.

[Patryk27]

Status: got fixed some time ago (https://github.com/llvm/llvm-project/pull/145224), let me check if this change has already trickled down to rustc...

[Patryk27]

... not yet, backport ready (https://github.com/rust-lang/llvm-project/pull/184) - I'll re-ping once rustc has this merged.