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

[benshi001]

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]

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.

[benshi001]

In D96853#2577133, @aykevl wrote:

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]

fixes https://bugs.llvm.org/show_bug.cgi?id=31568

[Anastasia]

In D96853#2577172, @benshi001 wrote:

In D96853#2577133, @aykevl wrote:

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

?

[benshi001]

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.

[Anastasia]

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]

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]

This comment was removed by benshi001.

[benshi001]

In D96853#2659266, @Anastasia wrote:

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]

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.

[Anastasia]

In D96853#2660443, @benshi001 wrote:

In D96853#2659266, @Anastasia wrote:

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]

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]

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]

In D96853#2661168, @Anastasia wrote:

In D96853#2660443, @benshi001 wrote:

In D96853#2659266, @Anastasia wrote:

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 .

[Anastasia]

LGTM!

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 def err_verify_inconsistent_diags : Error<
     "'%0' diagnostics %select{expected|seen}1 but not %select{seen|expected}1: "
     "%2">;
 def err_verify_invalid_no_diags : Error<
     "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
     "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
     "no expected directives found: consider use of 'expected-no-diagnostics'">;
+def err_verify_nonconst_addrspace : Error<
+  "qualifier 'const' is needed for variables in address space '%0'">;
 
 def note_fixit_applied : Note<"FIX-IT applied suggested code changes">;
 def note_fixit_in_macro : Note<
     "FIX-IT unable to apply suggested code changes in a macro">;
 def note_fixit_failed : Note<
     "FIX-IT unable to apply suggested code changes">;
 def note_fixit_unfixed_error : Note<"FIX-IT detected an error it cannot fix">;
 def warn_fixit_no_changes : Note<

clang/lib/Basic/Targets/AVR.cpp

 }
 
 void AVRTargetInfo::getTargetDefines(const LangOptions &Opts,
                                      MacroBuilder &Builder) const {
   Builder.defineMacro("AVR");
   Builder.defineMacro("__AVR");
   Builder.defineMacro("__AVR__");
   Builder.defineMacro("__ELF__");
+  Builder.defineMacro("__flash", "__attribute__((address_space(1)))");
 
   if (!this->CPU.empty()) {
     auto It = llvm::find_if(
         AVRMcus, [&](const MCUInfo &Info) { return Info.Name == this->CPU; });
 
     if (It != std::end(AVRMcus))
       Builder.defineMacro(It->DefineName);
   }
 }

clang/lib/CodeGen/TargetInfo.cpp

 //===----------------------------------------------------------------------===//
 
 namespace {
 class AVRTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   AVRTargetCodeGenInfo(CodeGenTypes &CGT)
       : TargetCodeGenInfo(std::make_unique<DefaultABIInfo>(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule &CGM,
+                                  const VarDecl *D) const override {
+    // Check if a global/static variable is defined within address space 1
+    // but not constant.
+    LangAS AS = D->getType().getAddressSpace();
+    if (isTargetAddressSpace(AS) && toTargetAddressSpace(AS) == 1 &&
+        !D->getType().isConstQualified())
+      CGM.getDiags().Report(D->getLocation(),
+                            diag::err_verify_nonconst_addrspace)
+          << "__flash";
+    return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);
+  }
+
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
                            CodeGen::CodeGenModule &CGM) const override {
     if (GV->isDeclaration())
       return;
     const auto *FD = dyn_cast_or_null<FunctionDecl>(D);
     if (!FD) return;
     auto *Fn = cast<llvm::Function>(GV);
 

clang/test/CodeGen/address-space-avr.c

 // RUN: %clang_cc1 -triple avr -emit-llvm < %s | FileCheck %s
 
-// Test that function declarations in nonzero address spaces without prototype
-// are called correctly.
+// CHECK: @var0 {{.*}} addrspace(1) constant [3 x i16]
+// CHECK: @bar.var2 {{.*}} addrspace(1) constant [3 x i16]
+// CHECK: @var1 {{.*}} addrspace(1) constant [3 x i16]
 
 // CHECK: define{{.*}} void @bar() addrspace(1)
-// CHECK: call addrspace(1) void bitcast (void (...) addrspace(1)* @foo to void (i16) addrspace(1)*)(i16 3)
+// CHECK: call addrspace(1) void bitcast (void (...) addrspace(1)* @foo to void (i16) addrspace(1)*)
 // CHECK: declare void @foo(...) addrspace(1)
+
+__flash const int var0[] = {999, 888, 777};
+__flash static const int var1[] = {111, 222, 333};
+
+int i;
+
 void foo();
-void bar(void) {
-  foo(3);
+
+void bar() {
+  static __flash const int var2[] = {555, 666, 777};
+  foo(var1[i]);
 }

clang/test/CodeGen/avr-flash.c

+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(void) {
+  static __flash int b[] = {4, 6}; // expected-error {{qualifier 'const' is needed for variables in address space '__flash'}}
+  return b[0];
+}