[AVR] Initial AVR support #74818

carlos4242 · 2024-06-28T16:48:41Z

This adds support for the AVR target to Swift. It is a preliminary draft PR for discussion.
@rauhul @phausler @kubamracek

Note: names like "Embedded" I created many years ago, when it was only us using this code. This would probably not make sense now, but it gives you ideas of what we have been using and is a kick off point for discussion.

(See also #74817)

harlanhaskins · 2024-06-29T01:26:09Z

Would AVR need its own platform name or would the architecture conditional suffice? Embedded Swift is usually platform-agnostic (if we managed to compile a tiny Linux kernel for AVR, we would write Swift for this OS via #if os(Linux) && arch(avr))

rauhul · 2024-06-29T01:34:13Z

Would AVR need its own platform name or would the architecture conditional suffice? Embedded Swift is usually platform-agnostic (if we managed to compile a tiny Linux kernel for AVR, we would write Swift for this OS via #if os(Linux) && arch(avr))

It should not, the standard conditionals should suffice. I think when building for these targets we will use one or more triple from the following set: avr-{unknown, apple, atmel, swift}-none-{elf, macho?}.

The vendor part will be interesting to figure out.

lib/Basic/LangOptions.cpp

carlos4242 · 2024-06-30T16:39:28Z

@rauhul @harlanhaskins ... they sound like good ideas.

Would AVR need its own platform name or would the architecture conditional suffice? Embedded Swift is usually platform-agnostic (if we managed to compile a tiny Linux kernel for AVR, we would write Swift for this OS via #if os(Linux) && arch(avr))

It should not, the standard conditionals should suffice. I think when building for these targets we will use one or more triple from the following set: avr-{unknown, apple, atmel, swift}-none-{elf, macho?}.

The vendor part will be interesting to figure out.

To be honest, when I made these changes, I was partly copying other people's PRs where they were adding architectures. All I really wanted was to be able to add -target avr ... to the swiftc command line and have it work, i.e. still compile at least as far as bitcode, passing the target triple down to llvm. I think the llvm back end probably pretty much ignores everything after the architecture, because it's not relevant on that architecture, you're always building bare metal.

I'd be completely happy with avr-none-none or avr-unknown-unknown as target triples. What do you guys think are the minimal changes to get that to work? I can strip out everything else.

(Subarch and vendor are irrelevant on AVR.)

harlanhaskins · 2024-06-30T16:44:59Z

I think all you really need is to make sure the compiler recognizes AVR as an architecture, and that AVR implies 16 bit. Then you just need to tell the embedded seam routines to build a swift standard library for 16 bit AVR, and then clients will just be able to compile for AVR, import the standard library, and manually link/search for Arduino headers

harlanhaskins · 2024-06-30T16:45:57Z

If we did want specific support for Arduino as a named platform, along with searching for those headers and writing an overlay, for example, that could be done. I suspect that should be done outside of this PR though, and I'm curious if @rauhul has more opinions on how to do that.

carlos4242 · 2024-07-01T10:11:50Z

I think all you really need is to make sure the compiler recognizes AVR as an architecture, and that AVR implies 16 bit. Then you just need to tell the embedded seam routines to build a swift standard library for 16 bit AVR, and then clients will just be able to compile for AVR, import the standard library, and manually link/search for Arduino headers

Actually we are quite separate from the Arduino platform. Most code written for our platform doesn’t use Arduino headers or libraries at all. We quite often use avr libc which is a much older and more widely used set of libraries. But again it’s really optional. A lot of us are writing pure Swift code with our own pure Swift hardware libraries. The only thing that might be needed to build valid binaries are some custom linker scripts. But that’s only if we want to go as far as having Swift do the full link. For now I think a great first step would be just compiling object files. In fact probably just getting to the Bitcode stage would be enough. In our own tooling we only ever use Swift to compile to Bitcode and then let other tools take over from there.

kubamracek · 2024-07-02T21:20:16Z

I suggest we define a first goal to just accept AVR as a target CPU in the compiler and be able to start the compilation. For that we will need some parts of this PR, plus some more (hopefully trivial) changes. I think we should drop from this PR everything that's not the minimal set of changes to get there. (Or open a new PR and leave this as a reference.)

Specifically, I think we'll need:

SwiftSetIfArchBitness.cmake change from this PR
SupportedConditionalCompilationArches change in LangOptions.cp from this PR
Enabling the AVR LLVM target in standard LLVM builds for Swift in build-presets.ini (look for llvm-targets-to-build)
A lit test that compiles something trivial in -parse-stdlib mode for AVR.
Optionally, if this this ends up needing a lot more work then let's make that a separate follow PR:
- Enabling the build of the embedded stdlib for AVR in stdlib/public/CMakeLists.txt + a lit test compiling in non -parse-stdlib mode

carlos4242 · 2024-07-02T23:14:34Z

I'm a little bit guessing with build-presets.ini as it's a bit opaque to me. Feel free to tweak anything. I didn't have time to write tests yet as it's after midnight and I have to get up for work tomorrow. I'll try to pick it up again tomorrow or Thursday.

kubamracek · 2024-07-03T02:38:32Z

@swift-ci please smoke test

lib/Basic/LangOptions.cpp

kubamracek · 2024-07-03T03:24:36Z

Actually, we'll also need to fix the default set of llvm-targets-to-build in utils/build_swift/build_swift/driver_arguments.py.

carlos4242 · 2024-07-03T08:47:41Z

I'd appreciate some help/advice how to fix the broken test? https://ci.swift.org/job/swift-PR-macos-smoke-test/14213/consoleFull#-1588501433d6fdb6cb-f376-4f2e-8bce-d31c7304698b

It looks like it's not emitting the expected fixit and is instead suggesting avr as a possible fixit, which I think is not what we would like (I don't mind if swift suggests arm, or x86 or riscv or avr... so long as these more rare architectures are further down the list). Could someone suggest how I might fix this please?

rauhul · 2024-07-03T15:45:12Z

I'd appreciate some help/advice how to fix the broken test? https://ci.swift.org/job/swift-PR-macos-smoke-test/14213/consoleFull#-1588501433d6fdb6cb-f376-4f2e-8bce-d31c7304698b

It looks like it's not emitting the expected fixit and is instead suggesting avr as a possible fixit, which I think is not what we would like (I don't mind if swift suggests arm, or x86 or riscv or avr... so long as these more rare architectures are further down the list). Could someone suggest how I might fix this please?

It's an interesting question, I'm not sure what architecture the fixit should suggest. It appears to suggest bad options in either case (arm or avr) and it seems like suggesting an arch based Levenshtein distance would be a much better UX.

@DougGregor do you happen to have opinions on behavior of the target conditional fixits with the addition of the AVR architecture? For reference the lit errors are below:

/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Parse/ConditionalCompilation/basicParseErrors.swift:90:10: error: unexpected note produced: did you mean 'avr'?
#if arch(leg) // expected-warning {{unknown architecture for build configuration 'arch'}} expected-note{{did you mean 'arm'?}} {{10-13=arm}}
         ^
/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Parse/ConditionalCompilation/basicParseErrors.swift:126:11: error: unexpected note produced: did you mean 'avr'?
#if !arch(tecture) && !os(ystem) && !_endian(ness)
          ^
/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Parse/ConditionalCompilation/basicParseErrors.swift:144:4: error: expected note not produced
// expected-note@-5 {{did you mean 'arm'?}} {{26-29=arm}}
~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Parse/ConditionalCompilation/basicParseErrors.swift:145:23: error: incorrect message found
// expected-note@-6 {{did you mean 'i386'?}} {{26-29=i386}}
                      ^~~~~~~~~~~~~~~~~~~~
                      did you mean 'avr'?

DougGregor · 2024-07-03T16:34:34Z

It's an interesting question, I'm not sure what architecture the fixit should suggest. It appears to suggest bad options in either case (arm or avr) and it seems like suggesting an arch based Levenshtein distance would be a much better UX.

@DougGregor do you happen to have opinions on behavior of the target conditional fixits with the addition of the AVR architecture? For reference the lit errors are below:

It is using Levenshtein distance. I think the test here is unfortunate and the easiest thing to fix. The edit distance between leg and arm is 3, as it is with avr, which is why we're seeing the test failure here.

I recommend updating the test to be more realistic: replace leg and tecture with arn, which has an edit distance of 1 to arm and is therefore more robust against adding new architectures.

rauhul · 2024-07-03T16:36:13Z

It's an interesting question, I'm not sure what architecture the fixit should suggest. It appears to suggest bad options in either case (arm or avr) and it seems like suggesting an arch based Levenshtein distance would be a much better UX.
@DougGregor do you happen to have opinions on behavior of the target conditional fixits with the addition of the AVR architecture? For reference the lit errors are below:

It is using Levenshtein distance. I think the test here is unfortunate and the easiest thing to fix. The edit distance between leg and arm is 3, as it is with avr, which is why we're seeing the test failure here.

I recommend updating the test to be more realistic: replace leg with arn, which has an edit distance of 1 and is therefore more robust against adding new architectures.

Serves me right for not reading the implementation before typing, thanks for the correction! I think updating the test with arn is a really good idea.

lib/Basic/LangOptions.cpp

kubamracek · 2024-07-03T16:46:08Z

@swift-ci please smoke test

carlos4242 · 2024-07-03T20:53:27Z

Yes. When I’m trying to avoid register tearing things like that I suppress interrupts and re-enable them after. It’s all very basic. They are very simple processors!If it’s best to remove this from the PR then I’m happy to. Whatever is best?On 3 Jul 2024, at 18:06, Rauhul Varma ***@***.***> wrote: @rauhul commented on this pull request. In lib/Basic/LangOptions.cpp:

@@ -391,6 +392,10 @@ void LangOptions::setHasAtomicBitWidth(llvm::Triple triple) {

setMaxAtomicBitWidth(128); break; + case llvm::Triple::ArchType::avr: + setMaxAtomicBitWidth(0); if I recall correctly there are no atomic instructions, but you can mask interrupts do some ops and unmask —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

carlos4242 · 2024-07-03T20:55:37Z

Sounds like a good idea. CheersOn 3 Jul 2024, at 17:35, Doug Gregor ***@***.***> wrote: It's an interesting question, I'm not sure what architecture the fixit should suggest. It appears to suggest bad options in either case (arm or avr) and it seems like suggesting an arch based Levenshtein distance would be a much better UX. @DougGregor do you happen to have opinions on behavior of the target conditional fixits with the addition of the AVR architecture? For reference the lit errors are below: It is using Levenshtein distance. I think the test here is unfortunate and the easiest thing to fix. The edit distance between leg and arm is 3, as it is with avr, which is why we're seeing the test failure here. I recommend updating the test to be more realistic: replace leg with arn, which has an edit distance of 1 and is therefore more robust against adding new architectures. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

carlos4242 · 2024-07-03T21:44:42Z

It’s more of a todo.But you have to be pretty careful on AVR. The back end handles some things for you. For example in some cases when you read or change a 16 bit register you must read and write the bytes in a specific order to avoid the possibility of tearing if interrupts are enabled.But I think you don’t normally get CAS instructions on most AVR chips, that kind of thing. I think.I wonder if llvm atomic instructions may not lower to anything meaningful.I’m happy to either remove this change and see if it causes trouble (we can always easily add it later). Or if you guys prefer I can check through the tablegen etc to get a definitive answer what atomic IR ops do?On 3 Jul 2024, at 17:43, Kuba (Brecka) Mracek ***@***.***> wrote: @kubamracek commented on this pull request. In lib/Basic/LangOptions.cpp:

@@ -391,6 +392,10 @@ void LangOptions::setHasAtomicBitWidth(llvm::Triple triple) {

setMaxAtomicBitWidth(128); break; + case llvm::Triple::ArchType::avr: + setMaxAtomicBitWidth(0); Is this a "TODO, deal with this later" or is there really no atomics on AVR? Any details about this? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

carlos4242 · 2024-07-04T23:59:13Z

@rauhul @kubamracek I think I fixed the broken unit test, at least it works locally, I also added a very basic parse test that should check the architecture exists and a couple of basic features, modelled on wasm. And suggested a tiny documentation tweak. I hope this is useful.

rauhul · 2024-07-05T00:00:34Z

@swift-ci please smoke test

DougGregor · 2024-07-05T15:18:13Z

@swift-ci please smoke test

DougGregor · 2024-07-05T17:37:31Z

@swift-ci please smoke test

DougGregor · 2024-07-05T17:42:57Z

@swift-ci please smoke test

DougGregor · 2024-07-05T18:12:33Z

@swift-ci please smoke test Linux

carlos4242 · 2024-07-05T21:10:08Z

All tests passed do you want to review it now?

cmake/modules/SwiftSetIfArchBitness.cmake

kubamracek · 2024-07-05T21:27:59Z

Other than the small note, this looks good to me to go in. If you don't mind, could you wait with merging until let's say Tuesday to see if there's any more review comments from others (it's a Friday after a holiday in the US)?

kubamracek · 2024-07-05T21:31:22Z

Oh and one more thing: Because we went back and forth on some of the changes, I'd suggest squashing the commits before merging and also expanding on the commit message to make it more clear that this isn't actually yet a meaningful AVR support, but just the first step towards it.

carlos4242 · 2024-07-06T00:10:21Z

Sounds like a good idea. Also the squash and clarify that this is a first basic step. On 5 Jul 2024, at 22:28, Kuba (Brecka) Mracek ***@***.***> wrote: Other than the small note, this looks good to me to go in. If you don't mind, could you wait with merging until let's say Tuesday to see if there's any more review comments from others (it's a Friday after a holiday in the US)? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

carlos4242 · 2024-07-06T00:11:12Z

Ok. I’ll take a look. I must admit I’m not strong on cmake!On 5 Jul 2024, at 22:25, Kuba (Brecka) Mracek ***@***.***> wrote: @kubamracek commented on this pull request. In cmake/modules/SwiftSetIfArchBitness.cmake:

@@ -6,7 +6,9 @@ function(set_if_arch_bitness var_name)

"" # multi-value args ${ARGN}) - if("${SIA_ARCH}" STREQUAL "i386" OR + if("${SIA_ARCH}" STREQUAL "avr") + set("${var_name}" "${SIA_CASE_16_BIT}" PARENT_SCOPE) This probably also needs an extension on line 5, otherwise I think it's not doing anything? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

- Add simple support for the AVR architecture, as a supported conditional compilation value, and added to the default llvm targets to build. (Later PRs will fix support for 16-bit pointers, which is broken in places, and any fixes needed to get the standard library to build.) (Note: AVR as a target is expected to always be compiled with -enable-experimental-feature Embedded.)

carlos4242 · 2024-07-06T17:25:27Z

OK @kubamracek. I think I've addressed all your concerns. This seems like a good first PR now.

kubamracek · 2024-07-07T02:14:54Z

@swift-ci please test

rauhul

Step 1 ✅

carlos4242 · 2024-07-08T13:08:06Z

Hi @kubamracek @rauhul should we tag anyone else who needs to review this? p.s. I don't think I have permission to merge it anyway, so someone else will have to do that when it's time.

DougGregor · 2024-07-08T17:39:01Z

Let's go ahead and merge this. Thank you @carlos4242!

kubamracek · 2024-07-08T19:35:49Z

🎉

benrimmington reviewed Jun 29, 2024

View reviewed changes

lib/Basic/LangOptions.cpp Outdated Show resolved Hide resolved

carlos4242 mentioned this pull request Jun 30, 2024

16 bit pointer fixes #74693

Draft

rauhul mentioned this pull request Jul 2, 2024

Add support for AVR #74899

Open

9 tasks

kubamracek mentioned this pull request Jul 2, 2024

Improve support in Swift for program address space N in llvm DataLayout #74817

Closed

carlos4242 force-pushed the add-target-avr branch from 1a58252 to f0f8782 Compare July 2, 2024 23:12

kubamracek requested review from phausler, kubamracek, rauhul, eeckstein and MaxDesiatov July 3, 2024 02:38

rauhul reviewed Jul 3, 2024

View reviewed changes

lib/Basic/LangOptions.cpp Outdated Show resolved Hide resolved

kubamracek reviewed Jul 3, 2024

View reviewed changes

lib/Basic/LangOptions.cpp Outdated Show resolved Hide resolved

carlos4242 requested review from hamishknight and rintaro as code owners July 4, 2024 23:57

carlos4242 requested review from kubamracek, rauhul and benrimmington July 5, 2024 13:03

kubamracek reviewed Jul 5, 2024

View reviewed changes

cmake/modules/SwiftSetIfArchBitness.cmake Outdated Show resolved Hide resolved

kubamracek approved these changes Jul 5, 2024

View reviewed changes

carlos4242 force-pushed the add-target-avr branch from 36df990 to ee57481 Compare July 6, 2024 17:22

carlos4242 changed the title ~~Simple AVR support~~ [AVR] Initial AVR support Jul 6, 2024

carlos4242 requested a review from kubamracek July 6, 2024 17:25

kubamracek approved these changes Jul 7, 2024

View reviewed changes

rauhul approved these changes Jul 7, 2024

View reviewed changes

DougGregor merged commit fbc4e67 into swiftlang:main Jul 8, 2024
5 checks passed

[AVR] Initial AVR support #74818

[AVR] Initial AVR support #74818

Uh oh!

Conversation

carlos4242 commented Jun 28, 2024

Uh oh!

harlanhaskins commented Jun 29, 2024

Uh oh!

rauhul commented Jun 29, 2024

Uh oh!

Uh oh!

carlos4242 commented Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harlanhaskins commented Jun 30, 2024

Uh oh!

harlanhaskins commented Jun 30, 2024

Uh oh!

carlos4242 commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubamracek commented Jul 2, 2024

Uh oh!

carlos4242 commented Jul 2, 2024

Uh oh!

kubamracek commented Jul 3, 2024

Uh oh!

Uh oh!

kubamracek commented Jul 3, 2024

Uh oh!

carlos4242 commented Jul 3, 2024

Uh oh!

rauhul commented Jul 3, 2024

Uh oh!

DougGregor commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rauhul commented Jul 3, 2024

Uh oh!

Uh oh!

kubamracek commented Jul 3, 2024

Uh oh!

carlos4242 commented Jul 3, 2024 via email

Uh oh!

carlos4242 commented Jul 3, 2024 via email

Uh oh!

carlos4242 commented Jul 3, 2024 via email

Uh oh!

carlos4242 commented Jul 4, 2024

Uh oh!

rauhul commented Jul 5, 2024

Uh oh!

DougGregor commented Jul 5, 2024

Uh oh!

DougGregor commented Jul 5, 2024

Uh oh!

DougGregor commented Jul 5, 2024

Uh oh!

DougGregor commented Jul 5, 2024

Uh oh!

carlos4242 commented Jul 5, 2024

Uh oh!

Uh oh!

kubamracek commented Jul 5, 2024

Uh oh!

kubamracek commented Jul 5, 2024

Uh oh!

carlos4242 commented Jul 6, 2024 via email

Uh oh!

carlos4242 commented Jul 6, 2024 via email

Uh oh!

carlos4242 commented Jul 6, 2024

Uh oh!

kubamracek commented Jul 7, 2024

Uh oh!

rauhul left a comment

Choose a reason for hiding this comment

Uh oh!

carlos4242 commented Jul 8, 2024

Uh oh!

carlos4242 commented Jun 30, 2024 •

edited

Loading

carlos4242 commented Jul 1, 2024 •

edited

Loading

DougGregor commented Jul 3, 2024 •

edited

Loading