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

Add compiler support for AVR architecture (Arduino) #14393

Merged

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Mar 25, 2024

Experimental support for the AVR (Atmel) CPU architectures as found on popular Arduino boards (e.g. Uno).

  • Supports the avr-unknown-unknown target triple;
  • Enables the AVR target in LLVM;
  • Implements the ABI as per GCC AVR (barely tested), with support for both regular and "reduced tiny" cores (less registers);
  • Starts hacking support for 16-bit size_t extracted as Add Program#size_t and Target#size_bit_width #14442;
  • Requires CPU model to be specified with --mcpu (impacts the ABI, codegen & linker);
  • Exports the specified CPU model as a compiler flag;
  • Links the executable program (courtesy of @RX14);
  • No support for stdlib (must compile with --prelude=empty), for obvious reasons. Alternative stdlib shall happen in external shards.

Initial cross compilations seem to generate a proper file, identical or close to clang and less optimized for size than AVR GCC.

Follow ups:

  • Add support for -Os (optsize) and -Oz (minsize) with LLVMPassBuilderOptionsSetInlinerThreshold. The thresholds in LLVM are 50 and 5 respectively (I guess clang uses these) while Rust uses 75 and 25. We could also disable some specific optimizations (e.g. loop unrolling). That would help reduce the program size which can be very restricted on AVR (e.g. 32KB). See Add compiler flags -Os and -Oz to optimize binary size #14463.

  • The compiler hardcodes pointer sizes to 64-bit (Pointer#address, #object_id and pointer manipulation). It would be interesting to have per-target support for size_t and uintptr_t actual sizes, instead of casting and assuming LLVM will optimize.

Basic instructions:

Install the GCC AVR toolchain and avr-libc. On Debian/Ubuntu:

$ apt install gcc-avr avr-libc

Cross compile a crystal binary, specifying the actual CPU (e.g. Arduino Uno uses an atmega328p) and the empty prelude, link it as a .elf file and generate a .hex file:

$ CC=avr-gcc crystal build \
  --target=avr-unknown-unknown --mcpu=atmega328p \
  --prelude=empty --no-debug --single-module -O2 blink.cr
$ avr-objcopy -O ihex blink.elf blink.hex

When #14463 is merged we may use -Os and -Oz to further reduce the executable file size. The --gc-sections linker option is mostly useful when linking external libraries.

Now you should be able to upload the hex file to your AVR board, using avrdude for example.

src/llvm/abi/avr.cr Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor Author

I broke the compiler with the llvm.memset.* change:

Module validation failed: Call parameter type does not match function signature!
i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64)
 i64  call void @llvm.memset.p0.i64(ptr align 4 %0, i8 0, i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64), i1 false), !dbg !45
Call parameter type does not match function signature!
i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64)
 i64  call void @llvm.memset.p0.i64(ptr align 4 %0, i8 0, i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64), i1 false), !dbg !68
Call parameter type does not match function signature!
  %4 = sext i32 %3 to i64, !dbg !152
 i64  call void @llvm.memset.p0.i64(ptr %1, i8 %value, i64 %4, i1 false), !dbg !153
Call parameter type does not match function signature!
  %10 = mul i64 ptrtoint (ptr getelementptr (i8, ptr null, i32 1) to i64), %9, !dbg !158
 i64  call void @llvm.memset.p0.i64(ptr align 4 %11, i8 0, i64 %10, i1 false), !dbg !158
Call parameter type does not match function signature!
i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64)
 i64  call void @llvm.memset.p0.i64(ptr align 4 %0, i8 0, i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64), i1 false), !dbg !164
Call parameter type does not match function signature!
i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64)
 i64  call void @llvm.memset.p0.i64(ptr align 4 %0, i8 0, i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64), i1 false), !dbg !256
Call parameter type does not match function signature!
i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64)
 i64  call void @llvm.memset.p0.i64(ptr align 4 %0, i8 0, i64 ptrtoint (ptr getelementptr (%"Slice(UInt8)", ptr null, i32 1) to i64), i1 false), !dbg !269

That may be what broke CI :p

@straight-shoota
Copy link
Member

We should try to pull out changes that are not directly affecting AVR into separate PRs. That's mostly the refactor around size_t I suppose.

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 4, 2024

The interpreter specs broke on CI with a linker error:

cannot load /__w/crystal/crystal/src/llvm/ext/llvm_ext.o (/__w/crystal/crystal/src/llvm/ext/llvm_ext.o: cannot open shared object file: No such file or directory)

I'm not sure what's happening?

@HertzDevil
Copy link
Contributor

You need:

require "spec"

{% if flag?(:interpreted) && !flag?(:win32) %}
  # TODO: figure out how to link against libstdc++ in interpreted code (#14398)
  pending LLVM::ABI::AVR
  {% skip_file %}
{% end %}

require "llvm"

# ...

@ysbaddaden
Copy link
Contributor Author

Thanks @HertzDevil !

@ysbaddaden
Copy link
Contributor Author

I extracted the size_t patches into its own pull request, and rebased this branch off that new pull request, to clean it up and keep it focused on the AVR target.

Support for -Os and -Oz will come in their own PR.

@ysbaddaden ysbaddaden marked this pull request as ready for review April 6, 2024 08:59
@HertzDevil
Copy link
Contributor

Does flag?(:bits8) or flag?(:bits16) hold for AVR?

@ysbaddaden
Copy link
Contributor Author

@HertzDevil I guess it depends on what we want bitsN to mean: are the bits the register size or the usual pointer bit width. I'd say the later?

@straight-shoota
Copy link
Member

It's implemented via Target#pointer_bit_width, so I'd assume that to be the intention. However, we haven't really had distinctive values yet, so it could just be underspecified.

Anyway, the meaning of bitN flags feels like a separate discussion. We might even want to enhance/replace them with more precise flags that allow a distinction between size and address types.

@ysbaddaden
Copy link
Contributor Author

I'm running some tests to pass structs (seems to be OK) as well as pointers to structs, and I feel like passing pointers isn't working or it's accessing Pointer(T)#value or both. The generated assembly is much more complex than it should be (like it's copying the structs).

There might be something wrong in the ABI.

@ysbaddaden
Copy link
Contributor Author

I don't see an issue in the LLVM IR codegen related to pointers. However the memset call when initializing a struct is wrong: it should call the i16 variant but it's calling the i64 one 🤔

call addrspace(1) void @llvm.memset.p0.i64(ptr noundef nonnull align 1 dereferenceable(16) %.fca.3.insert.fca.2.gep, i8 0, i64 16, i1 false)

The %.fca.X.insert.fca.Y.gep names are weird (clang uses regular %x) but maybe that's normal.

@ysbaddaden
Copy link
Contributor Author

Weird, the c_memset_fun in codegen is using the proper @llvm.memset.p0.i16 name but the LLVM IR contains the i64 one. We might be using Intrinsics.memset somewhere but since :bits64 is false it should default to the i32 version 😕

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 8, 2024

The non optimized LLVM IR (-O0) properly declares and uses @llvm.memset.p0.i16, while the optimization pass eventually removes it and adds a call to @llvm.memset.p0.i64 🤷

The generated assembly is close to the one generated by clang (again: much less optimized than avr gcc).

I guess it was all a non issue.

@ysbaddaden
Copy link
Contributor Author

I changed Codegen#set_alignment to no longer hardcode the alignment to either 8 or 4, in order to return 1 for the AVR target.

I'll extract the change to its own PR because I'm wondering if we shouldn't delegate to the ABI to get the proper alignment. For example its always 1 for AVR while we still get pointless N bytes alignments for chars, ints and floats... which may waste some space (the ATmega328 only has 2KB of SRAM).

@HertzDevil
Copy link
Contributor

I think you could try querying the "preferrred alignment" instead of the "ABI alignment" since it seems Clang sets it up that way. On the other hand that would increase double's alignment from 4 to 8 for 32-bit x86

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 8, 2024

@HertzDevil I'll look into the "preferred alignment", and yes, the data layout for AVR is to align everything to 8-bit / 1-byte, same as AVR GCC. The i386 change means that the change indeed warrants its own PR.

Apparently clang overrides the data layout to have 4 byte alignment for double:
https://github.com/llvm/llvm-project/blob/2bc637b1ce935550b6e09618c76474253943a7cc/clang/lib/Basic/Targets/X86.h#L442-L453 (I'm not sure what's the layout we use).

@ysbaddaden
Copy link
Contributor Author

I reverted the alignment as it was only used for atomic load & store instructions that don't exist on AVR. Let's tackle that later in its own PR.

@RX14
Copy link
Contributor

RX14 commented Apr 20, 2024

I tested this myself, and wrote a little code on an attiny202, which has only 2kb of flash and 128 bytes of ram! Here's a video of blinking lights.

Add support for the `avr-unknown-unknown` target to the compiler, which
enables the LLVM AVR target when available (should be compiled in by
default).

Implement the ABI, following the AVR GCC call convention, hence extern C
libraries must be compiled with AVR GCC (clang is buggy when passing of
returning some structs).

The ABI is experimental, and only basic calls have been verified against
the same calls made by AVR GCC so far. Both regular and "reduced tiny"
cores (with less registers) are supported.
This allows to know actual support for some features, for example which
pins are available.
Don't hardcode the alignment to 4 or 8 when the actual value depends on
the actual target (e.g. it's always 1 for AVR targets).
- Automatically link the executable program;
- Set file extension to ".elf" instead of none;
- Require CPU model (--mcpu) that impacts the ABI, codegen & linker
@ysbaddaden
Copy link
Contributor Author

Rebased from master (without any squash) to remove the initial commit that was an older version of #14442 that got merged, and now created a conflict.

spec/std/llvm/avr_spec.cr Show resolved Hide resolved
src/compiler/crystal/semantic/flags.cr Show resolved Hide resolved
src/llvm/abi/avr.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.13.0 milestone Apr 29, 2024
src/llvm/abi/avr.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
@straight-shoota straight-shoota changed the title Feature: AVR architecture (Arduino) Add compiler support for AVR architecture (Arduino) May 2, 2024
@straight-shoota straight-shoota merged commit 7ecb968 into crystal-lang:master May 2, 2024
60 checks passed
@ysbaddaden ysbaddaden deleted the feature/avr-architecture branch May 2, 2024 18:57
@crysbot
Copy link

crysbot commented May 14, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/real-stuff-written-in-crystal/6840/12

@crysbot
Copy link

crysbot commented May 14, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/real-stuff-written-in-crystal/6840/13

@robacarp
Copy link
Contributor

@ysbaddaden wow! I have been hoping for this for years! 🚀

@crysbot
Copy link

crysbot commented Sep 3, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/crystal-lang-for-microcontrollers/7165/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants