Page MenuHomePhabricator

[clang][AVR] Support variable decorator '__flash'
ClosedPublic

Authored by benshi001 on Feb 17 2021, 3:13 AM.

Diff Detail

Event Timeline

benshi001 created this revision.Feb 17 2021, 3:13 AM
benshi001 requested review of this revision.Feb 17 2021, 3:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2021, 3:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This patch immitates avr-gcc behavior on __flash

  1. __flash variables are put into .section .progmem.data
  2. __flash non-const variables are rejected.
aykevl added a comment.EditedFeb 20 2021, 5:03 PM

I am not very familiar with Clang so I can't say much about it. Although I wonder whether a macro is the right way to implement this? Is there something similar in other targets? (GPUs tend to have lots of address spaces, you could take a look there).

clang/test/CodeGen/address-space-avr.c
2–5

This comment is now out of date.

I am not very familiar with Clang so I can't say much about it. Although I wonder whether a macro is the right way to implement this? Is there something similar in other targets? (GPUs tend to have lots of address spaces, you could take a look there).

A macro definition is the simplest way, an alias to __attribute__((address_space(0))). I do not find any other easy way for a plain keyword __flash.

benshi001 updated this revision to Diff 325285.Feb 21 2021, 5:39 AM
benshi001 updated this revision to Diff 325286.Feb 21 2021, 5:45 AM
benshi001 marked an inline comment as done.
benshi001 updated this revision to Diff 327361.Mar 1 2021, 10:57 PM

I am not very familiar with Clang so I can't say much about it. Although I wonder whether a macro is the right way to implement this? Is there something similar in other targets? (GPUs tend to have lots of address spaces, you could take a look there).

A macro definition is the simplest way, an alias to __attribute__((address_space(0))). I do not find any other easy way for a plain keyword __flash.

Ok, macro could be a reasonable direction. This is how clang implements address spaces for CUDA. Another option is adding a keyword. OpenCL implements address spaces this way. You can't add a target-specific keyword but considering that it start with "__" it should be reasonable to add this to C/C++ mode. You could for example align with gcc implementation.

I am guessing this will only be supported by 1 target in clang? Then target address spaces make sense otherwise you can also extend the language address space enum. Are you also planning to add __flashN?
https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

clang/include/clang/Basic/DiagnosticSemaKinds.td
2995 ↗(On Diff #327361)

Minor wording change:

qualifier '%select{const|volatile}0' is %select{needed|invalid}1

Could you not use the same parameter in select twice i.e.

qualifier '%select{const|volatile}0' is %select{needed|invalid}0

?

I am guessing this will only be supported by 1 target in clang? Then target address spaces make sense otherwise you can also extend the language address space enum. Are you also planning to add __flashN?
https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

Yes. I will add __flashN in another patch.

benshi001 marked an inline comment as done.

Btw is any pointer conversion between address space 1 and the default address space allowed? I.e. is the following code valid:

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

or

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo((int*)var1);

?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2996 ↗(On Diff #333659)

FYI I guess you don't need 3 parameters any more so it should be:

"for variables in address space '%1'">;
clang/lib/CodeGen/TargetInfo.cpp
8120

ok, now you should only need:

<< 0 <<  "__flash";
clang/test/CodeGen/avr-flash.c
2

If you are only checking the diagnostics you should pass:

-emit-llvm-only -> -syntax-only

and also it should be moved to clang/test/Sema.

5

Let's also test the volatile case.

benshi001 added inline comments.Mar 30 2021, 6:42 PM
clang/test/CodeGen/avr-flash.c
2

This test can not run with -syntax-only. Since the check is performed in the codegen stage.

I would like to do the check in the Sema stage, but there is no target specific code in the sema stage. And such checks are better to be put into AVRTargetCodeGenInfo.

benshi001 updated this revision to Diff 334330.Mar 30 2021, 8:11 PM
This comment was removed by benshi001.

Btw is any pointer conversion between address space 1 and the default address space allowed? I.e. is the following code valid:

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

or

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo((int*)var1);

?

No. clang denied with error: passing 'const __attribute__((address_space(1))) int *' to parameter of type 'const int *' changes address space of pointer while avr-gcc accepted it.

It seems I need more efforts to make clang-avr fully compatible with avr-gcc.

benshi001 marked 3 inline comments as done.Mar 30 2021, 8:35 PM
benshi001 added inline comments.
clang/test/CodeGen/avr-flash.c
2

I haved moved all __flash related to the CodeGen part, now it has nothing to do with Sema/-syntax-only.

5

I have removed the volatile diagnose message, since avr-gcc has no restriction on that.

benshi001 marked an inline comment as done.Mar 30 2021, 8:38 PM
benshi001 updated this revision to Diff 334336.Mar 30 2021, 8:48 PM

Btw is any pointer conversion between address space 1 and the default address space allowed? I.e. is the following code valid:

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

or

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo((int*)var1);

?

No. clang denied with error: passing 'const __attribute__((address_space(1))) int *' to parameter of type 'const int *' changes address space of pointer while avr-gcc accepted it.

It seems I need more efforts to make clang-avr fully compatible with avr-gcc.

Ok, it makes sense for clang to align with gcc in general but do you know whether or where this conversions are described? By default if not specified explicitly implicit conversions are always disallowed in ISO/IEC DTR 18037 and explicit casts are always allowed but can yeild undefined behavior if address spaces don't overlap.

Anastasia added inline comments.Mar 31 2021, 5:37 AM
clang/include/clang/Basic/DiagnosticFrontendKinds.td
162

Ok, do you plan to pass anything other than __flash here? If not then you don't need to pass an argument in this diagnostic.

clang/test/CodeGen/avr-flash.c
2

I would like to do the check in the Sema stage, but there is no target specific code in the sema stage. And such checks are better to be put into AVRTargetCodeGenInfo

Sema has target hooks and access to the target specific address spaces so you could indeed move them to Sema. Normally we do diagnostics as early as possible but there is no strict rule and there are some tradeoffs. So, you can also keep it here to allow better code incapsulation and simplify maintenance.

If you need to implement the pointer conversion logic you might move this completely to Sema. Btw just to highlight the following change https://reviews.llvm.org/D62574 that generalizes some logic that might be useful for your pointer conversions.

6

Is there anything you are testing with these array subscript expressions? If not I suggest removing them and only keep what is needed for testing the new functionality.

benshi001 marked an inline comment as done.Mar 31 2021, 7:26 AM
benshi001 added inline comments.
clang/include/clang/Basic/DiagnosticFrontendKinds.td
162

Yes. There may be __flash1 ... __flash5 in the future.

clang/test/CodeGen/avr-flash.c
2

Thanks. I will try your way.

benshi001 marked an inline comment as done.Apr 4 2021, 6:40 AM

Btw is any pointer conversion between address space 1 and the default address space allowed? I.e. is the following code valid:

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

or

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo((int*)var1);

?

No. clang denied with error: passing 'const __attribute__((address_space(1))) int *' to parameter of type 'const int *' changes address space of pointer while avr-gcc accepted it.

It seems I need more efforts to make clang-avr fully compatible with avr-gcc.

Ok, it makes sense for clang to align with gcc in general but do you know whether or where this conversions are described? By default if not specified explicitly implicit conversions are always disallowed in ISO/IEC DTR 18037 and explicit casts are always allowed but can yeild undefined behavior if address spaces don't overlap.

I do not think clang needs to fully follow avr-gcc's way. Since avr-gcc generates wrong assembly that c code.

__flash static const int var1[] = {111, 222, 333};
void foo(int*);
....
foo(var1);

LDD (which is used access data space) is generated in th function foo, while LPM is needed for accessing var1 (in the program space).
avr-gcc silently accepts the c code with any warning and generates wrong assembly. And I think clang should rise an error .

This revision is now accepted and ready to land.Apr 6 2021, 11:00 AM
This revision was automatically updated to reflect the committed changes.