Page MenuHomePhabricator

[clang][AVR] Implement '__flashN' for variables on different flash banks
ClosedPublic

Authored by benshi001 on Dec 17 2021, 9:58 PM.

Diff Detail

Event Timeline

benshi001 created this revision.Dec 17 2021, 9:58 PM
benshi001 requested review of this revision.Dec 17 2021, 9:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2021, 9:58 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
benshi001 updated this revision to Diff 395284.Dec 18 2021, 5:08 AM
benshi001 updated this revision to Diff 395809.Dec 22 2021, 1:26 AM

avr-gcc checks whether the device supports the flash bank used. For example:

$ cat test.c
int d = 5;
const int ro = 5;
__flash const int f = 5;
__flash1 const int f1 = 5;
__flash2 const int f2 = 5;

$ avr-gcc -mmcu=attiny84 -Os -c -o test.o test.c
test.c:4:20: error: variable ‘f1’ located in address space ‘__flash1’ beyond flash of 64 KiB
 __flash1 const int f1 = 5;
                    ^
test.c:5:20: error: variable ‘f2’ located in address space ‘__flash2’ beyond flash of 64 KiB
 __flash2 const int f2 = 5;
                    ^

It does not appear that this patch has a similar check, while I think that would be useful. Or did you leave it out intentionally?

benshi001 updated this revision to Diff 397279.Jan 4 2022, 6:29 AM
benshi001 added a comment.EditedJan 4 2022, 6:36 AM

avr-gcc checks whether the device supports the flash bank used. For example:

$ cat test.c
int d = 5;
const int ro = 5;
__flash const int f = 5;
__flash1 const int f1 = 5;
__flash2 const int f2 = 5;

$ avr-gcc -mmcu=attiny84 -Os -c -o test.o test.c
test.c:4:20: error: variable ‘f1’ located in address space ‘__flash1’ beyond flash of 64 KiB
 __flash1 const int f1 = 5;
                    ^
test.c:5:20: error: variable ‘f2’ located in address space ‘__flash2’ beyond flash of 64 KiB
 __flash2 const int f2 = 5;
                    ^

It does not appear that this patch has a similar check, while I think that would be useful. Or did you leave it out intentionally?

I have updated my patch, in the newly added test clang/test/Sema/avr-flash.c, all __flash1/__flash2/__flash3/__flash4/__flash5 are denied when running clang against at90s8515 (which belongs to the avr-2 family).

I intentionally using at90s8515 instead of attiny84 in my test case, due to the compiler generates wrong code (use of register r0~r15 can be generated) for the avrtiny family. Actually I plan to

  1. deny compiling c code for the avr-0 family, and only assembly code is accepted by clang, that behavior matches avr-gcc's.
  2. Temporarily disable compiling c code for the avr-tiny family, report an error like "c code is not supported on avrtiny currently", until the support of avr-tiny is fully done.
benshi001 updated this revision to Diff 397287.Jan 4 2022, 6:55 AM
benshi001 added a comment.EditedJan 4 2022, 7:03 AM

avr-gcc checks whether the device supports the flash bank used. For example:

$ cat test.c
int d = 5;
const int ro = 5;
__flash const int f = 5;
__flash1 const int f1 = 5;
__flash2 const int f2 = 5;

$ avr-gcc -mmcu=attiny84 -Os -c -o test.o test.c
test.c:4:20: error: variable ‘f1’ located in address space ‘__flash1’ beyond flash of 64 KiB
 __flash1 const int f1 = 5;
                    ^
test.c:5:20: error: variable ‘f2’ located in address space ‘__flash2’ beyond flash of 64 KiB
 __flash2 const int f2 = 5;
                    ^

It does not appear that this patch has a similar check, while I think that would be useful. Or did you leave it out intentionally?

Sorry, it might be that I have misunderstood your concern. Yes, I did add a test for checking the rejection of unsupported flash banks, as shown in the test file clang/test/Sema/avr-flash.c.

The best error message should be something like address space ‘__flash2’ is not supported on current device, but actually the real error in my patch is unknown type name '__flash2', so I write a TOTO line for that

// TODO: It would be better to report "'__flash5' is not supported on at908515".

I will try to fix it in my future patches, and current patch implements

  1. support __flashN on different devices, and deny __flashN that beyond the real flash size of user specified device (though the error message is inexact)
  2. generate corresponding address space attributes for __flashN

So I hope current patch can be accepted and I will fix the inaccurate error message in the future.

aykevl accepted this revision.Jan 18 2022, 6:48 AM

Looks good to me!

clang/lib/Basic/Targets/AVR.cpp
27

This works and is fine, but I think you could also name it NumFlashBanks so that it is the number of flash banks supported on the device (0 if LPM/ELPM is not supported). With 0 for the attiny11 (which has no accessible flash banks) and 1 for the attiny22 (because it has only one flash bank that can be accessed: flash bank 0).

Just a suggestion, the current system looks good to me.

clang/lib/CodeGen/TargetInfo.cpp
8290

These hardcoded numbers are a bit unfortunate, but OK.

clang/test/Sema/avr-flash.c
11

I think this can be implemented by always setting the __flash* defines and checking whether they are valid for the current device in getGlobalVarAddressSpace.

This revision is now accepted and ready to land.Jan 18 2022, 6:48 AM
benshi001 marked an inline comment as done.Jan 19 2022, 2:21 AM
benshi001 added inline comments.
clang/lib/Basic/Targets/AVR.cpp
27

I will change it to NumFlashBanks when committing.

benshi001 marked 2 inline comments as done.Jan 19 2022, 2:27 AM
benshi001 added inline comments.
clang/test/Sema/avr-flash.c
11

Maybe it is better to do in getGlobalVarAddressSpace for reporting the exact source position.

a.c:1:20: error: variable ‘arr’ located in address space ‘__flash5’ beyond flash of 64 KiB
 __flash5 int const arr[8];

I will try it in the next patch.

benshi001 marked an inline comment as done.Jan 19 2022, 2:28 AM