llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.85k stars 11.47k forks source link

[AVR] Microchip changed the semantics of 16 bit register writes on newer cores #58395

Closed G33KatWork closed 1 year ago

G33KatWork commented 1 year ago

When accessing 16 bit registers like counters on an 8 bit AVR core, these accesses have to be split into two 8 bit register writes. To do this, the peripherals have a TEMP register that the CPU automatically writes to on the first write and a write to the other half of the target register yields a write of this temporary register and the currently written value into the 16 bit target register in the same cycle.

On old cores like an ATmega8, the datasheet states that you need to write the high byte first and then the low byte. See ATmega8 datasheet, page 78.. LLVM takes care of this here for writes and here for reads.

However, newer cores like the tinyAVR 1-series like an ATtiny817 require these writes to happen with the LOW byte first, followed by the high byte. See ATtiny817 datasheet, page 55. I didn't check any other datasheets yet about which other cores might be affected by this change.

I just ran into this when trying to use Rust on an ATtiny817. Writing to the counter register with a u16 seemingly caused the low byte to be ignored or otherwise wrong all the time. Once I converted my writes manually to two u8 writes in the order specified by the datasheet, things started to work. Flipping them once more showed the same broken results as the single u16 write.

I also confirmed the wrong order by disassembling the resulting Rust program and comparing it to an equivalent C program compiled by avr-gcc. The avr-gcc compiled program was equivalent except for the write order of the two register halves.

G33KatWork commented 1 year ago

I did some poking a the avr-gcc code to figure out which devices are affected by this change. Apparently all XMEGA cores require the low byte to be written first. Here are a few functions in gcc responsible for this:

And here is a list of all the XMEGA cores.

Maybe that helps a bit.

benshi001 commented 1 year ago

fixed by https://reviews.llvm.org/D141752

sprintersb commented 1 year ago

fixed by https://reviews.llvm.org/D141752

Not completely, take for example

volatile int* stv (int volatile *p, int volatile *q)
{
    *q = 0;
    *p++ = 0;
    return p;
}

compiled with -S -mmcu=avr5 -Os. Output is:

stv:                                    ; @stv
; %bb.0:
    movw    r26, r24
    ldi r24, 0
    ldi r25, 0
    movw    r30, r22
    st  Z, r24
    std Z+1, r25
    st  X+, r24
    st  X+, r25
    movw    r24, r26
    ret
I didn't try all addressing modes, but at least the above two (indirect and post-increment) are not correct, because we have to obey the following access orders for multi-byte volatile data: non-Xmega Xmega
Load low first low first
Store high first low first

For an explanation, see for example Does avr-gcc properly work with 16-bit AVR I/O registers? on stackoverflow.

A device is Xmega iff __AVR_ARCH__ >= 100.

--version
Ubuntu clang version 16.0.0 (++20230118052744+29d25f9e9a4c-1~exp1~20230118172840.512)
benshi001 commented 1 year ago

I have uploaded a new version of my patch https://reviews.llvm.org/D141752, your new test is correctly compiled to

        .text
.set __tmp_reg__, 0
.set __zero_reg__, 1
.set __SREG__, 63
.set __SP_H__, 62
.set __SP_L__, 61
        .file   "a.c"
        .globl  stv                             ; -- Begin function stv
        .p2align        1
        .type   stv,@function
stv:                                    ; @stv
; %bb.0:
        ldi     r18, 0
        ldi     r19, 0
        movw    r30, r22
        std     Z+1, r19
        st      Z, r18
        movw    r30, r24
        std     Z+1, r19
        st      Z, r18
        adiw    r24, 2
        ret
.Lfunc_end0:
        .size   stv, .Lfunc_end0-stv
                                        ; -- End function
        .addrsig