[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In https://reviews.llvm.org/D35082#887855, @rjmccall wrote:

> Why is most of this patch necessary under the design of adding a non-canonical __private address space?


There are two reasons that we need a flag to indicate an address space is simplicit:

1. We need a consistent way to print the address space qualifier depending on whether it is implicit or not.

We only print address space qualifier when it is explicit. This is not just for private address space. It is for all
address spaces.

2. In some rare situations we need to know whether an address space is implicit when doing the semantic

checking.

Since the implicit property is per address space qualifier, we need this flag to be on the qualifier.



================
Comment at: include/clang/AST/Type.h:336
+  /// space makes difference.
+  bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+  void setImplicitAddressSpaceFlag(bool Value) {
----------------
rjmccall wrote:
> isAddressSpaceImplicit()
Will change.


================
Comment at: include/clang/AST/Type.h:337
+  bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+  void setImplicitAddressSpaceFlag(bool Value) {
+    Mask = (Mask & ~IMask) | (((uint32_t)Value) << IShift);
----------------
rjmccall wrote:
> setAddressSpaceImplicit
Will change.


================
Comment at: lib/AST/ItaniumMangle.cpp:2232
       case LangAS::opencl_constant: ASString = "CLconstant"; break;
+      case LangAS::opencl_private:  ASString = "CLprivate";  break;
       case LangAS::opencl_generic:  ASString = "CLgeneric";  break;
----------------
rjmccall wrote:
> In what situation is this mangled?  I thought we agree this was non-canonical.
OpenCL has overloaded builtin functions, e.g. `__attribute__((overloadable)) void f(private int*)` and  `__attribute__((overloadable)) void f(global int*)`. These functions need to be mangled so that the mangled names are different.


================
Comment at: test/SemaOpenCL/storageclass.cl:63
+  static float l_implicit_static_var = 0;          // expected-error {{variables in function scope cannot be declared static}}
+  static constant float l_constant_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+  static global float l_global_static_var = 0;     // expected-error {{variables in function scope cannot be declared static}}
----------------
Anastasia wrote:
> Does it make sense to put different address spaces here since this code is rejected earlier anyway?
In Sema I saw code handling different address spaces in various places. I want to make sure that all address spaces are handled correctly.


https://reviews.llvm.org/D35082