This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Rework MCU family detection to support more AVR MCUs
Needs RevisionPublic

Authored by vlastik on Apr 1 2020, 8:09 AM.

Details

Summary

AVR port now supports only ATmega328.
This patch makes more MCUs work with clang

Diff Detail

Event Timeline

vlastik created this revision.Apr 1 2020, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 8:09 AM
vlastik updated this revision to Diff 254456.Apr 2 2020, 2:06 AM

Fix regression introduced by patch

Failed unit tests:

  • Clang.Driver::avr-link-mcu-family-unimplemented.c
  • Clang.Misc::target-invalid-cpu-note.c
dylanmckay requested changes to this revision.Apr 18 2020, 11:33 PM
dylanmckay added inline comments.
clang/lib/Basic/Targets/AVR.h
21

Order this #include alphabetically with the ones above it

llvm/include/llvm/Support/AVRTargetParser.h
9

Add another sentence or two to this documentation, as this will be a new public-facing API that all frontends can use.

llvm/lib/Support/AVRTargetParser.cpp
20

Add a comment like FIXME: At some point, this list should removed and the logic directly driven from the definitions in AVRDevices.td`.

This revision now requires changes to proceed.Apr 18 2020, 11:33 PM
aykevl added a subscriber: aykevl.Apr 19 2020, 2:50 PM

Great! I was considering writing something like this but this patch is much better than what I had in mind.

Note that this might conflict with D78117.

aykevl set the repository for this revision to rG LLVM Github Monorepo.
aykevl added inline comments.
clang/lib/Basic/Targets/AVR.cpp
35

This should have tests. Take a look at D78117 for how you can add them.

Patch generally looks good, but I agree that it could use some basic tests. You don't need to do specifically test all 20 million releases, but make sure you cover the first and last in the array as well as something in the middle, just to make sure your scans are working right.

llvm/include/llvm/Support/AVRTargetParser.h
27

I think the relationship would be clearer if this type were named MCUFamilyInfo.

More generally, these types and fields could use some comments, like "Information about a specific AVR microcontroller release" or "Information about a family of AVR microcontrollers" or "Some sort of number that goes somewhere and has some kind of meaning, maybe it's reported by hardware or something, I dunno".

aykevl added a comment.EditedApr 29 2020, 5:13 PM

A thought: should the AVR family be included in the target triple? Like avr5-unknown-unknown (similar to armv6m-none-eabi, armv7m-none-eabi, etc).
The different variations do sometimes change the architecture in incompatible ways, such as with the call instruction which pushes a return address of two or three bytes depending on the architecture variant (and thus makes it impossible to pass parameters on the stack reliably).

I don't think this should block this patch (which I hope will get updated so it can land soon), but it may be something to consider.

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 5:13 PM

@aykevl

A thought: should the AVR family be included in the target triple? Like avr5-unknown-unknown (similar to armv6m-none-eabi, armv7m-none-eabi, etc).
The different variations do sometimes change the architecture in incompatible ways, such as with the call instruction which pushes a return address of two or three bytes depending on the architecture variant (and thus makes it impossible to pass parameters on the stack reliably).

I've thought about this a few years ago, I suspect it is not possible. It seems like we really do need to compile a sysroot/stdlib/runtime libraries for each and every distinct MCU.

https://github.com/rust-lang/rust/issues/44036#issuecomment-324330757

Does each device have a subtly different ISA?

tl;dr Sometimes, yes

In general, all devices can be _more or less_ grouped into families.

For example, avr1, avr2, avr25, ... avr5, avr51, are all family names. These families each define an subset of the full AVR ISA.

You can find the AVR backend's definition of these families in AVRDevices.td. You can see that in general, the families build up on top of each other in terms of supported instructions/features.

The problem is that not every realised ISA can be cleanly separated into a family.

For example, the ATtiny26 is a part of the avr2 family, but it also happens to support the "LPMX" set of instructions

Another example is the ATMega8515, which _almost_ implements all of avr4, but not quite all, and so the linked definition bases the device off avr2 and adds the extra features explicitly.

Does the backend need to know about peripherals?

No

Or why do you want the LLVM backend to know when it's targeting, for example, "atmega328"?

Solely for deciding what subset of the ISA is supported.All device-specific information required (or used) by the backend can be found inside AVRDevices.td.

There is a bunch of other related discussion on https://github.com/rust-lang/rust/issues/44036 too.