15 ms·

AVR-GCC Compiler Makes Questionable Code

73 points by zdw 4y ago

joshvm 4y ago
The fact that avr-gcc exists at all is quite amazing though. It's allowed many people to get into embedded design without needing to purchase licenses for other proprietary compilers. It might not be the most optimal compiler, but sub-optimal does not mean incorrect. If you know enough to quibble with the code, and you're seeing performance issues like this, you can just inline your assembly into C. AVR has historically been one of the best routes to get into embedded work because there is solid open-source tooling, interfacing the hardware is simple, and it's very well documented in the open by Atmel/Microchip. Arduino would not exist without avr-gcc.
- bpye 4y ago
  I've hoped for a while that LLVM's support for such targets would improve, but maybe the ubiquity of ARM microcontrollers makes that more unlikely now. There is an AVR target for example, but in my attempt to use Rust on an AVR I ran into several incorrectness bugs - and newer versions of LLVM are plain broken [0]. There is also an MSP430 backend but I've never had an excuse to play around with that... Maybe the libgccjit backend for Rust will solve the specific scenario I was interested in, but it doesn't improve anything for LLVM itself. [0] - https://github.com/rust-lang/rust/issues/88252 https://github.com/rust-lang/rust/issues/88252
  - tuetuopay 4y ago
    Off topic, but I can confirm Rust's MSP430 backend works great.
  - bombela 4y ago
    Took a while but this issue was fixed this year. Until then, you could compile a patched version of LLVM, or use an older nightly. And the code generation was quite broken indeed. Most importantly the saving of registers within interrupt handlers. Hard to understand the bug, but I had documented a simple inline asm solution on the issue. It has been fixed since then. My last experience was quite good. And I didn't notice wrong codegen. As an aside, the inline asm combines pretty well with generics to produce custom machine code during compilation. For example I was able to reproduce the gcc-avr built-in for delays: https://github.com/avr-rust/delay/blob/cycacc/src/delay_cycles.rs https://github.com/avr-rust/delay/blob/cycacc/src/delay_cycl...
    - bpye 4y ago
      Oh hey, you're right, I went and found the issue but didn't scroll down after seeing it was still open. That's great! I might try and revive my previous project. I had tried an older nightly but that's where I think I was seeing bad codegen.
  - Gordonjcp 4y ago
    Isn't Rust designed around things like immutable variables, where you need to copy and copy and copy the same things over and over, and therefore expect to run it on something with infinite amounts of memory? It might be suitable for modern desktop computers which have hundreds or even thousands of kilobytes of memory available, but not a microcontroller with 2kB of RAM.
    - pix128 4y ago
      Maybe I'm dumb, but why would you need to copy an immutable value?
      - ItsHarper 4y ago
        Because the only way to have a modified version of that value is to make a brand new copy with the modification.
    - cylon13 4y ago
      This is incorrect
      - Gordonjcp 4y ago
        Which part is incorrect?
        arcticbull 4y ago
        It's not fair to say that rust is 'designed around immutable variables' - rust has move semantics, but move semantics just mean that if you have a variable, 'a' and you assign it to variable 'b', the variable 'a' is now dead and can no longer be referenced. This exists at the lexical level, it's a no-op in generated code. Is that maybe what you were referring to? For certain scalar types, flexibility is increased by allowing 'copy' semantics where assigning 'a' into 'b' makes 'b' a copy of 'a', and both are alive. Then it ends up mattering how heavy the type is - although you can only implement 'copy' for things you can trivially memcpy, so nothing on the heap. Generally anything that would be expensive to duplicate doesn't get 'copy' semantics, but instead requires you to move it into a new variable, or explicitly clone it. Rust also has immutable-by-default semantics, but it's by default. You can mutate the contents of structs, but there can only be one mutable reference XOR an arbitrary quantity of read-only references and aliasing is not permitted. This forms the basis for many of the safety guarantees. Did that help? I was guessing at what you meant, so if that wasn't it I can always try again. [edit] within the context of microcontrollers Rust requires you to be very explicit about what is and isn't permitted, and how things should work. You can disallow in your construction pretty much anything expensive or non-trivial.
3836293648 4y ago
How does this even happen? Shouldn't basically all the code that made these terrible decisions be shared between all backends? This should be done before it's lowered to any specific assembly language
- qayxc 4y ago
  > Shouldn't basically all the code that made these terrible decisions be shared between all backends? What makes you think this? The "middle-end" (in GCC-lingo) is where architecture-specific optimisations happen and that needs to be modified on a per-architecture basis. As an obvious example, not all CPUs have vector instructions, yet auto-vectorisation is part of the optimisation passes for architectures that support it. There's several ways of doing this, but the assembly-emitter (i.e. the "backend") is not the part concerned with that at all.
  - 3836293648 4y ago
    Because the issues aren't codegen, they're weird, broken optimisations. Loading values several times, subtracting zero. Why would gcc have several register allocators instead of one slightly more configurable one? Why aren't useless actions (subtract by zero) removed in generic optimisation? Of course there are architecture specific optimisations, but the examples provided here are not of that kind
    - qayxc 4y ago
      gcc is a complex beast and optimisation is hard. There's no such such thing as "generic optimisations" that remove useless actions, because depending on the hardware, certain seemingly useless actions are actually required to make correctness guarantees. Without access to the offending source code, one can only speculate what actually led to this result. Another problem is simply that there's probably only a handful of people, likely employed by a single company or two, who maintain the parts for this specific architecture. It's very likely that there are too few users who actually report this kind of issues and too few resources available to test and address them. It's not an issue with gcc or its optimiser in general.
bjt2n3904 4y ago
I actually have no idea who maintains AVR-GCC, but this seems like an excellent bug report that could improve the performance of a very important tool chain!
- psychphysic 4y ago
  5.4.0 is ancient I think.
  - qayxc 4y ago
    Amazing how in the world of software, 6 years is considered "ancient". I know the field is a fast moving one, but my goodness. The target architecture is used in the world of embedded hardware where product lifetimes often can be measured in decades (think locomotives, industrial installations, ships, etc.) For compliance and certification reasons alone you're often stuck with using older software versions. 6 years isn't actually that bad in that context. Keep in mind that many devices, such as ATMs, still use OS/2(!) or Windows 2000(!) as their operating systems, for example.
    - psychphysic 4y ago
      Very good point. Although my meaning was more that in the context of opening an issue on a project, this version is ancient.
    - Gibbon1 4y ago
      Well and it's not like WG14 has been doing anything constructive for the last 20 years. So it's not like there are any new features to take advantage of. Far as I can tell the only thing I get out of newer versions of gcc is more pointless warnings to silence.
- Teongot 4y ago
  An excellent bug report for a compiler would * have reduced source code for the example, preferably a single source file, in a form which can be compiled * have the exact command line used to build it * have the exact compiler version being used * apply to a supported version of the compiler (GCC 5 is from 2015...) * include an accurate explanation of what is wrong with the generated code Maybe the last one is covered by this blog post, but things like > I can maybe understand the subtraction of constant 0, if there’s another code path that jumps to 7ba6 suggests that the author hasn't fully analysed the generated code. The analysis is OK though and with source code it would be a pretty good bug report.
psychphysic 4y ago
It's a shame the author didn't include the C snippet so that we might verify this. Not that I'd be so surprised if avr-gcc doesn't produce spectacular code. Some of this is very surprising. How could the compiler function of it doesn't know which registers have which values? Unless it was emitting very simple chunks.
- dezgeg 4y ago
  In particular there seem to be couple of MMIO stores, maybe those are done with some accessor/macro which accidentally makes too much stuff volatile causing this extra zero-addition etc. So yes seeing all the code could give some clues...
- Beltalowda 4y ago
  He included some code in the comments: https://bigmessowires.com/static/avr-gcc-main.cpp https://bigmessowires.com/static/avr-gcc-main.cpp
maltaisn 4y ago
I kept a short list of the times I was disappointed by avr-gcc on my last project (11.1.0 mind you, not the ancient 5.4.0 one maintained by Microchip): Get highest bits with a right shift: https://godbolt.org/z/nqseMvMh5 https://godbolt.org/z/nqseMvMh5 - Expected: shift only highest byte of word (3 cycles). - Actual: all bytes of word are shifted several times (184 cycles, +18B). Copy low nibble to high nibble in bit field struct: https://godbolt.org/z/x8nrTWPja https://godbolt.org/z/x8nrTWPja - Expected: swap & or byte (4 cycles). - Actual: mul used to shift left (7 cycles, +6B). Extract bits 6:11 from an byte array encoding a 16 bit field: https://godbolt.org/z/EPTnbjGeP https://godbolt.org/z/EPTnbjGeP - Expected: build result into high byte by shifting bits 6:7 from low byte into high byte using rotate (11 cycles). - Actual: mul used to shift (16 cycles, +8B), or build result into low byte (13 cycles, +4B). Unrolled memcpy: https://godbolt.org/z/Yvbr7vT5a https://godbolt.org/z/Yvbr7vT5a - Expected: memcpy uses `ld X+` followed by `st Z+`. The same is expected here (20 cycles) - Actual: the compiler doesn't know about that the X register support post increment, creating `adiw`/`sbiw` chains instead (29 cycles, +18B). This is especially bad when the compiler is register starved when writing structs, it makes the program a lot larger than it should be. It's very easy to get a register starve situation: read from a pointer (register Z) to write a struct referred to by a pointer (X register), with both pointers originally stored on the stack (Y register). Oops, the struct uses the X register, every access generates 2x the code it should.
deleted 4y ago
[deleted]
russdill 4y ago
I found the default jump table implementation to be quite costly when using something with a very small flash size such as the attiny10. Luckily gcc now has goto label support for inline assembly. https://gist.github.com/russdill/abbacfe5dba1ba4070584efe44f46a58 https://gist.github.com/russdill/abbacfe5dba1ba4070584efe44f...
chadaustin 4y ago
I was also surprised by how suboptimal avr-gcc's generated code is. On the other hand, the AVR experience is reminiscent of early DOS, terrible C compilers and all, so it remains nostalgic! Two examples: https://gcc.godbolt.org/z/M4oasdK4a https://gcc.godbolt.org/z/M4oasdK4a * Population count: I haven't found a way to convince AVR-GCC to use LSR's shift into carry with ADC. * Strength reduction: AVR doesn't have a barrel shifter, so it's a huge win to incrementally shift during the loop. AVR-GCC doesn't see this.
codewiz 4y ago
> Most people believe that modern compilers generate better-optimized assembly code than humans, but look at this example from AVR-GCC 5.4.0 with -O2 optimization level Modern compilers? GCC 5.1.0 was released in 2015, and 5.4.0 is a bugfix release. Why would someone blog about its poor code generation without bothering to try the latest release? Besides, the AVR is an exceptional architecture: an 8-bit RISC with sizeof(int) == sizeof(short) == 2. Neither GCC nor C were designed for 8-bit processors.
- Beltalowda 4y ago
  People were saying "modern compilers generate better-optimized assembly code than humans" in 2015 too, and well before that. It's not that long ago. 5.4.0 is the version that ships with Debian and Ubuntu, so I guess that's why this particular version was used.
  - maxnoe 4y ago
    What do you mean with: "5.4 is the Version that ships with Ubuntu"? 5.4 was the default compiler on Ubuntu 16.04, which is end of life. Ubuntu 18.04: 7.4 Ubuntu 20.04: 9.3 Ubuntu 22.04: 11.2
    - Beltalowda 4y ago
      gcc-avr is a different package from the regular gcc: https://packages.debian.org/sid/gcc-avr https://packages.debian.org/sid/gcc-avr https://packages.ubuntu.com/search?suite=kinetic&searchon=names&keywords=gcc-avr https://packages.ubuntu.com/search?suite=kinetic&searchon=na... I don't know why it's an older version, you'll have to ask the Debian people. Maybe there's a reason, but most likely ist's justthat gcc tends to be a difficult piece of software to package and not enough people care/have time to keep the avr version to to date.
carapace 4y ago
Excuse me if this is a dumb question, is there something like Godbolt for AVR compilers?
- astrange 4y ago
  Godbolt* does support AVR gcc at least. * actually "compiler explorer"
  - carapace 4y ago
    Cheers
rurban 4y ago
Well, still much better than sdcc, which often fails to optimize array accesses on certain targets. And just recently I took acr-gcc down the - O3 -flto rabbit hole, and everything survived, and with the huge size win, I could add plenty of new functionality and checks.

73 points by zdw 4y ago

The fact that avr-gcc exists at all is quite amazing though. It's allowed many people to get into embedded design without needing to purchase licenses for other proprietary compilers. It might not be the most optimal compiler, but sub-optimal does not mean incorrect. If you know enough to quibble with the code, and you're seeing performance issues like this, you can just inline your assembly into C. AVR has historically been one of the best routes to get into embedded work because there is solid open-source tooling, interfacing the hardware is simple, and it's very well documented in the open by Atmel/Microchip. Arduino would not exist without avr-gcc.

bpye 4y ago
I've hoped for a while that LLVM's support for such targets would improve, but maybe the ubiquity of ARM microcontrollers makes that more unlikely now. There is an AVR target for example, but in my attempt to use Rust on an AVR I ran into several incorrectness bugs - and newer versions of LLVM are plain broken [0]. There is also an MSP430 backend but I've never had an excuse to play around with that... Maybe the libgccjit backend for Rust will solve the specific scenario I was interested in, but it doesn't improve anything for LLVM itself. [0] - https://github.com/rust-lang/rust/issues/88252 https://github.com/rust-lang/rust/issues/88252
- tuetuopay 4y ago
  Off topic, but I can confirm Rust's MSP430 backend works great.
- bombela 4y ago
  Took a while but this issue was fixed this year. Until then, you could compile a patched version of LLVM, or use an older nightly. And the code generation was quite broken indeed. Most importantly the saving of registers within interrupt handlers. Hard to understand the bug, but I had documented a simple inline asm solution on the issue. It has been fixed since then. My last experience was quite good. And I didn't notice wrong codegen. As an aside, the inline asm combines pretty well with generics to produce custom machine code during compilation. For example I was able to reproduce the gcc-avr built-in for delays: https://github.com/avr-rust/delay/blob/cycacc/src/delay_cycles.rs https://github.com/avr-rust/delay/blob/cycacc/src/delay_cycl...
  - bpye 4y ago
    Oh hey, you're right, I went and found the issue but didn't scroll down after seeing it was still open. That's great! I might try and revive my previous project. I had tried an older nightly but that's where I think I was seeing bad codegen.
- Gordonjcp 4y ago
  Isn't Rust designed around things like immutable variables, where you need to copy and copy and copy the same things over and over, and therefore expect to run it on something with infinite amounts of memory? It might be suitable for modern desktop computers which have hundreds or even thousands of kilobytes of memory available, but not a microcontroller with 2kB of RAM.
  - pix128 4y ago
    Maybe I'm dumb, but why would you need to copy an immutable value?
    - ItsHarper 4y ago
      Because the only way to have a modified version of that value is to make a brand new copy with the modification.
  - cylon13 4y ago
    This is incorrect
    - Gordonjcp 4y ago
      Which part is incorrect?
      - arcticbull 4y ago
        It's not fair to say that rust is 'designed around immutable variables' - rust has move semantics, but move semantics just mean that if you have a variable, 'a' and you assign it to variable 'b', the variable 'a' is now dead and can no longer be referenced. This exists at the lexical level, it's a no-op in generated code. Is that maybe what you were referring to? For certain scalar types, flexibility is increased by allowing 'copy' semantics where assigning 'a' into 'b' makes 'b' a copy of 'a', and both are alive. Then it ends up mattering how heavy the type is - although you can only implement 'copy' for things you can trivially memcpy, so nothing on the heap. Generally anything that would be expensive to duplicate doesn't get 'copy' semantics, but instead requires you to move it into a new variable, or explicitly clone it. Rust also has immutable-by-default semantics, but it's by default. You can mutate the contents of structs, but there can only be one mutable reference XOR an arbitrary quantity of read-only references and aliasing is not permitted. This forms the basis for many of the safety guarantees. Did that help? I was guessing at what you meant, so if that wasn't it I can always try again. [edit] within the context of microcontrollers Rust requires you to be very explicit about what is and isn't permitted, and how things should work. You can disallow in your construction pretty much anything expensive or non-trivial.

3836293648 4y ago

How does this even happen? Shouldn't basically all the code that made these terrible decisions be shared between all backends? This should be done before it's lowered to any specific assembly language

qayxc 4y ago
> Shouldn't basically all the code that made these terrible decisions be shared between all backends? What makes you think this? The "middle-end" (in GCC-lingo) is where architecture-specific optimisations happen and that needs to be modified on a per-architecture basis. As an obvious example, not all CPUs have vector instructions, yet auto-vectorisation is part of the optimisation passes for architectures that support it. There's several ways of doing this, but the assembly-emitter (i.e. the "backend") is not the part concerned with that at all.
- 3836293648 4y ago
  Because the issues aren't codegen, they're weird, broken optimisations. Loading values several times, subtracting zero. Why would gcc have several register allocators instead of one slightly more configurable one? Why aren't useless actions (subtract by zero) removed in generic optimisation? Of course there are architecture specific optimisations, but the examples provided here are not of that kind
  - qayxc 4y ago
    gcc is a complex beast and optimisation is hard. There's no such such thing as "generic optimisations" that remove useless actions, because depending on the hardware, certain seemingly useless actions are actually required to make correctness guarantees. Without access to the offending source code, one can only speculate what actually led to this result. Another problem is simply that there's probably only a handful of people, likely employed by a single company or two, who maintain the parts for this specific architecture. It's very likely that there are too few users who actually report this kind of issues and too few resources available to test and address them. It's not an issue with gcc or its optimiser in general.

bjt2n3904 4y ago

I actually have no idea who maintains AVR-GCC, but this seems like an excellent bug report that could improve the performance of a very important tool chain!

psychphysic 4y ago
5.4.0 is ancient I think.
- qayxc 4y ago
  Amazing how in the world of software, 6 years is considered "ancient". I know the field is a fast moving one, but my goodness. The target architecture is used in the world of embedded hardware where product lifetimes often can be measured in decades (think locomotives, industrial installations, ships, etc.) For compliance and certification reasons alone you're often stuck with using older software versions. 6 years isn't actually that bad in that context. Keep in mind that many devices, such as ATMs, still use OS/2(!) or Windows 2000(!) as their operating systems, for example.
  - psychphysic 4y ago
    Very good point. Although my meaning was more that in the context of opening an issue on a project, this version is ancient.
  - Gibbon1 4y ago
    Well and it's not like WG14 has been doing anything constructive for the last 20 years. So it's not like there are any new features to take advantage of. Far as I can tell the only thing I get out of newer versions of gcc is more pointless warnings to silence.
Teongot 4y ago
An excellent bug report for a compiler would * have reduced source code for the example, preferably a single source file, in a form which can be compiled * have the exact command line used to build it * have the exact compiler version being used * apply to a supported version of the compiler (GCC 5 is from 2015...) * include an accurate explanation of what is wrong with the generated code Maybe the last one is covered by this blog post, but things like > I can maybe understand the subtraction of constant 0, if there’s another code path that jumps to 7ba6 suggests that the author hasn't fully analysed the generated code. The analysis is OK though and with source code it would be a pretty good bug report.

psychphysic 4y ago

It's a shame the author didn't include the C snippet so that we might verify this. Not that I'd be so surprised if avr-gcc doesn't produce spectacular code. Some of this is very surprising. How could the compiler function of it doesn't know which registers have which values? Unless it was emitting very simple chunks.

dezgeg 4y ago
In particular there seem to be couple of MMIO stores, maybe those are done with some accessor/macro which accidentally makes too much stuff volatile causing this extra zero-addition etc. So yes seeing all the code could give some clues...
Beltalowda 4y ago
He included some code in the comments: https://bigmessowires.com/static/avr-gcc-main.cpp https://bigmessowires.com/static/avr-gcc-main.cpp

maltaisn 4y ago

I kept a short list of the times I was disappointed by avr-gcc on my last project (11.1.0 mind you, not the ancient 5.4.0 one maintained by Microchip): Get highest bits with a right shift: https://godbolt.org/z/nqseMvMh5 https://godbolt.org/z/nqseMvMh5 - Expected: shift only highest byte of word (3 cycles). - Actual: all bytes of word are shifted several times (184 cycles, +18B). Copy low nibble to high nibble in bit field struct: https://godbolt.org/z/x8nrTWPja https://godbolt.org/z/x8nrTWPja - Expected: swap & or byte (4 cycles). - Actual: mul used to shift left (7 cycles, +6B). Extract bits 6:11 from an byte array encoding a 16 bit field: https://godbolt.org/z/EPTnbjGeP https://godbolt.org/z/EPTnbjGeP - Expected: build result into high byte by shifting bits 6:7 from low byte into high byte using rotate (11 cycles). - Actual: mul used to shift (16 cycles, +8B), or build result into low byte (13 cycles, +4B). Unrolled memcpy: https://godbolt.org/z/Yvbr7vT5a https://godbolt.org/z/Yvbr7vT5a - Expected: memcpy uses `ld X+` followed by `st Z+`. The same is expected here (20 cycles) - Actual: the compiler doesn't know about that the X register support post increment, creating `adiw`/`sbiw` chains instead (29 cycles, +18B). This is especially bad when the compiler is register starved when writing structs, it makes the program a lot larger than it should be. It's very easy to get a register starve situation: read from a pointer (register Z) to write a struct referred to by a pointer (X register), with both pointers originally stored on the stack (Y register). Oops, the struct uses the X register, every access generates 2x the code it should.

deleted 4y ago

[deleted]

russdill 4y ago

I found the default jump table implementation to be quite costly when using something with a very small flash size such as the attiny10. Luckily gcc now has goto label support for inline assembly. https://gist.github.com/russdill/abbacfe5dba1ba4070584efe44f46a58 https://gist.github.com/russdill/abbacfe5dba1ba4070584efe44f...

chadaustin 4y ago

I was also surprised by how suboptimal avr-gcc's generated code is. On the other hand, the AVR experience is reminiscent of early DOS, terrible C compilers and all, so it remains nostalgic! Two examples: https://gcc.godbolt.org/z/M4oasdK4a https://gcc.godbolt.org/z/M4oasdK4a * Population count: I haven't found a way to convince AVR-GCC to use LSR's shift into carry with ADC. * Strength reduction: AVR doesn't have a barrel shifter, so it's a huge win to incrementally shift during the loop. AVR-GCC doesn't see this.

codewiz 4y ago

> Most people believe that modern compilers generate better-optimized assembly code than humans, but look at this example from AVR-GCC 5.4.0 with -O2 optimization level Modern compilers? GCC 5.1.0 was released in 2015, and 5.4.0 is a bugfix release. Why would someone blog about its poor code generation without bothering to try the latest release? Besides, the AVR is an exceptional architecture: an 8-bit RISC with sizeof(int) == sizeof(short) == 2. Neither GCC nor C were designed for 8-bit processors.

Beltalowda 4y ago
People were saying "modern compilers generate better-optimized assembly code than humans" in 2015 too, and well before that. It's not that long ago. 5.4.0 is the version that ships with Debian and Ubuntu, so I guess that's why this particular version was used.
- maxnoe 4y ago
  What do you mean with: "5.4 is the Version that ships with Ubuntu"? 5.4 was the default compiler on Ubuntu 16.04, which is end of life. Ubuntu 18.04: 7.4 Ubuntu 20.04: 9.3 Ubuntu 22.04: 11.2
  - Beltalowda 4y ago
    gcc-avr is a different package from the regular gcc: https://packages.debian.org/sid/gcc-avr https://packages.debian.org/sid/gcc-avr https://packages.ubuntu.com/search?suite=kinetic&searchon=names&keywords=gcc-avr https://packages.ubuntu.com/search?suite=kinetic&searchon=na... I don't know why it's an older version, you'll have to ask the Debian people. Maybe there's a reason, but most likely ist's justthat gcc tends to be a difficult piece of software to package and not enough people care/have time to keep the avr version to to date.

carapace 4y ago

Excuse me if this is a dumb question, is there something like Godbolt for AVR compilers?

astrange 4y ago
Godbolt* does support AVR gcc at least. * actually "compiler explorer"
- carapace 4y ago
  Cheers

rurban 4y ago

Well, still much better than sdcc, which often fails to optimize array accesses on certain targets. And just recently I took acr-gcc down the - O3 -flto rabbit hole, and everything survived, and with the huge size win, I could add plenty of new functionality and checks.