[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4f173c0c42d0: [clang][AVR] Support variable decorator 
__flash (authored by benshi001).

Changed prior to commit:
  https://reviews.llvm.org/D96853?vs=334336=336593#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// 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];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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]);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -8108,6 +8108,19 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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 ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -158,6 +158,8 @@
 "%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<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// 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];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // RUN: %clang_cc1 -triple avr -emit-llvm < %s | FileCheck %s
 
-// Test that 

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 marked an inline comment as done.
benshi001 added a comment.

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 .


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 marked an inline comment as done.
benshi001 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:159
+def err_verify_nonconst_addrspace : Error<
+  "qualifier 'const' is needed for variables in address space '%0'">;
 

Anastasia wrote:
> 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.
Yes. There may be `__flash1 ... __flash5` in the future.



Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+

Anastasia wrote:
> benshi001 wrote:
> > benshi001 wrote:
> > > Anastasia wrote:
> > > > 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`.
> > > 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.
> > I haved moved all `__flash` related to the CodeGen part, now it has nothing 
> > to do with `Sema/-syntax-only`.
> > 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.
Thanks. I will try your way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:159
+def err_verify_nonconst_addrspace : Error<
+  "qualifier 'const' is needed for variables in address space '%0'">;
 

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.



Comment at: clang/test/CodeGen/avr-flash.c:5
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}

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.



Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+

benshi001 wrote:
> benshi001 wrote:
> > Anastasia wrote:
> > > 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`.
> > 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.
> I haved moved all `__flash` related to the CodeGen part, now it has nothing 
> to do with `Sema/-syntax-only`.
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 334336.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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]);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -8032,6 +8032,19 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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 ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -155,6 +155,8 @@
 "%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<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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 

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 marked 3 inline comments as done.
benshi001 added inline comments.



Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+

benshi001 wrote:
> Anastasia wrote:
> > 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`.
> 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.
I haved moved all `__flash` related to the CodeGen part, now it has nothing to 
do with `Sema/-syntax-only`.



Comment at: clang/test/CodeGen/avr-flash.c:4
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];

Anastasia wrote:
> Let's also test the `volatile` case.
I have removed the `volatile` diagnose message, since avr-gcc has no 
restriction on that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 334330.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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]);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -8032,6 +8032,18 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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 ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -155,6 +155,8 @@
 "%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<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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]
+// 

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added inline comments.



Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+

Anastasia wrote:
> 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`.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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);

?




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2996
+  "qualifier '%select{const|volatile}0' is %select{needed|invalid}0 "
+  "for variables in address space '%2'">;
 def err_compound_literal_with_address_space : Error<

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

```
  "for variables in address space '%1'">;
```



Comment at: clang/lib/CodeGen/TargetInfo.cpp:8045
+diag::err_qualifier_with_address_space)
+  << 0 << 0 << "__flash";
+return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);

ok,  now you should only need:

```
<< 0 <<  "__flash"; 
```



Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+

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`.



Comment at: clang/test/CodeGen/avr-flash.c:4
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];

Let's also test the `volatile` case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 333659.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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]);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
@@ -8032,6 +8033,19 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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_qualifier_with_address_space)
+  << 0 << 0 << "__flash";
+return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);
+  }
+
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2991,6 +2991,9 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_qualifier_with_address_space : Error<
+  "qualifier '%select{const|volatile}0' is %select{needed|invalid}0 "
+  "for variables in address space '%2'">;
 def err_compound_literal_with_address_space : Error<
   "compound literal in function scope may not be qualified with an address 
space">;
 def err_address_space_mismatch_templ_inst : Error<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.



> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2995
+def err_qualifier_with_address_space : Error<
+  "qualifier '%select{const|volatile}0' %select{needed|invalid}1 "
+  "for variables in address space '%2'">;

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
```
?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 327361.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,21 @@
 // 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]);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
@@ -8032,6 +8033,19 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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_qualifier_with_address_space)
+  << 0 << 0 << "__flash";
+return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);
+  }
+
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2991,6 +2991,9 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_qualifier_with_address_space : Error<
+  "qualifier '%select{const|volatile}0' %select{needed|invalid}1 "
+  "for variables in address space '%2'">;
 def err_compound_literal_with_address_space : Error<
   "compound literal in function scope may not be qualified with an address 
space">;
 def err_address_space_mismatch_templ_inst : Error<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
Index: clang/test/CodeGen/address-space-avr.c
===

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 325286.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+static __flash int arr[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+  return arr[i] + b[j];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -1,12 +1,15 @@
 // 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 i16 333
+// CHECK: @bar.var1 {{.*}} addrspace(1) constant i16 555
 // CHECK: define{{.*}} void @bar() addrspace(1)
 // CHECK: call addrspace(1) void bitcast (void (...) addrspace(1)* @foo to 
void (i16) addrspace(1)*)(i16 3)
 // CHECK: declare void @foo(...) addrspace(1)
+
+__flash const int var0 = 333;
+
 void foo();
 void bar(void) {
+   static __flash const int var1 = 555;
foo(3);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
@@ -8032,6 +8033,19 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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_qualifier_with_address_space)
+  << 0 << 0 << "__flash";
+return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);
+  }
+
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2991,6 +2991,9 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_qualifier_with_address_space : Error<
+  "qualifier '%select{const|volatile}0' %select{needed|invalid}1 "
+  "for variables in address space '%2'">;
 def err_compound_literal_with_address_space : Error<
   "compound literal in function scope may not be qualified with an address 
space">;
 def err_address_space_mismatch_templ_inst : Error<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+static __flash int arr[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+  return arr[i] + b[j];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- 

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 325285.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+static __flash int arr[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+  return arr[i] + b[j];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -3,10 +3,16 @@
 // Test that function declarations in nonzero address spaces without prototype
 // are called correctly.
 
+// CHECK: @var0 {{.*}} addrspace(1) constant i16 333
+// CHECK: @bar.var1 {{.*}} addrspace(1) constant i16 555
 // CHECK: define{{.*}} void @bar() addrspace(1)
 // CHECK: call addrspace(1) void bitcast (void (...) addrspace(1)* @foo to 
void (i16) addrspace(1)*)(i16 3)
 // CHECK: declare void @foo(...) addrspace(1)
+
+__flash const int var0 = 333;
+
 void foo();
 void bar(void) {
+   static __flash const int var1 = 555;
foo(3);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
@@ -8032,6 +8033,19 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule ,
+  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_qualifier_with_address_space)
+  << 0 << 0 << "__flash";
+return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);
+  }
+
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2991,6 +2991,9 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_qualifier_with_address_space : Error<
+  "qualifier '%select{const|volatile}0' %select{needed|invalid}1 "
+  "for variables in address space '%2'">;
 def err_compound_literal_with_address_space : Error<
   "compound literal in function scope may not be qualified with an address 
space">;
 def err_address_space_mismatch_templ_inst : Error<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+static __flash int arr[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+  return arr[i] + b[j];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ 

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.

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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ayke via Phabricator via cfe-commits]

aykevl added a comment.

I am not very familiar with Clang so I can't say much about it. Although I 
wonder whether the 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).




Comment at: clang/test/CodeGen/address-space-avr.c:3-4
 
 // Test that function declarations in nonzero address spaces without prototype
 // are called correctly.
 

This comment is now out of date.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.

This patch immitates avr-gcc behavior on `__flash`

1. `__flash` variables are put into `.section .progmem.data`
2. `__flash` non-const variables are rejected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96853/new/

https://reviews.llvm.org/D96853

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Ben Shi via Phabricator via cfe-commits]

benshi001 created this revision.
benshi001 added reviewers: dylanmckay, aykevl.
Herald added a subscriber: Jim.
benshi001 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96853

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Basic/Targets/AVR.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/avr-flash.c


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+static __flash int arr[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 
'const' needed for variables in address space '__flash'}}
+  return arr[i] + b[j];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
--- clang/test/CodeGen/address-space-avr.c
+++ clang/test/CodeGen/address-space-avr.c
@@ -3,10 +3,16 @@
 // Test that function declarations in nonzero address spaces without prototype
 // are called correctly.
 
+// CHECK: @var0 {{.*}} addrspace(1) constant i16 333
+// CHECK: @bar.var1 {{.*}} addrspace(1) constant i16 555
 // CHECK: define{{.*}} void @bar() addrspace(1)
 // CHECK: call addrspace(1) void bitcast (void (...) addrspace(1)* @foo to 
void (i16) addrspace(1)*)(i16 3)
 // CHECK: declare void @foo(...) addrspace(1)
+
+__flash const int var0 = 333;
+
 void foo();
 void bar(void) {
+   static __flash const int var1 = 555;
foo(3);
 }
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
@@ -8032,6 +8033,17 @@
   AVRTargetCodeGenInfo(CodeGenTypes )
   : TargetCodeGenInfo(std::make_unique(CGT)) {}
 
+  LangAS getGlobalVarAddressSpace(CodeGenModule , const VarDecl *D) const {
+// Check if a global/static variable is defined within address space 1
+// but not constant.
+if (toTargetAddressSpace(D->getType().getAddressSpace()) == 1 &&
+!D->getType().isConstQualified())
+  CGM.getDiags().Report(D->getLocation(),
+diag::err_qualifier_with_address_space)
+  << 0 << 0 << "__flash";
+return TargetCodeGenInfo::getGlobalVarAddressSpace(CGM, D);
+  }
+
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule ) const override {
 if (GV->isDeclaration())
Index: clang/lib/Basic/Targets/AVR.cpp
===
--- clang/lib/Basic/Targets/AVR.cpp
+++ clang/lib/Basic/Targets/AVR.cpp
@@ -308,6 +308,7 @@
   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(
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2991,6 +2991,9 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_qualifier_with_address_space : Error<
+  "qualifier '%select{const|volatile}0' %select{needed|invalid}1 "
+  "for variables in address space '%2'">;
 def err_compound_literal_with_address_space : Error<
   "compound literal in function scope may not be qualified with an address 
space">;
 def err_address_space_mismatch_templ_inst : Error<


Index: clang/test/CodeGen/avr-flash.c
===
--- /dev/null
+++ clang/test/CodeGen/avr-flash.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
+static __flash int arr[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+
+int foo(int i, int j) {
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' needed for variables in address space '__flash'}}
+  return arr[i] + b[j];
+}
Index: clang/test/CodeGen/address-space-avr.c
===
---