[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:9547-9555
+unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
+  // For OpenCL, the address space qualifier is 0 in AST.
+  if (AS == 0 && LangOpts.OpenCL)
+    return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
+    return AS;
+  else
----------------
t-tye wrote:
> Presumably this will not work if the user put an address space attribute on a variable with an address space of 0, since it is not possible to distinguish between a variable that never had an attribute and was function local, and one that has an explicit address space attribute specifying 0.
> 
> It would seem better if LangAS did not map the 0..LangAS::Offset to be target address spaces. Instead LangAS could use 0..LangAS::Count to be the CLANG explicitly specified values (reserving 0 to mean the default when none was specified), and values above LangAS::Count would map to the explicitly specified target address spaces. For example:
> 
> ```
> namespace clang {
>  
>  namespace LangAS {
>  
>  /// \brief Defines the set of possible language-specific address spaces.
>  ///
>  /// This uses values above the language-specific address spaces to denote
>  /// the target-specific address spaces biased by target_first.
>  enum ID {
>    default = 0,
>  
>    opencl_global,
>    opencl_local,
>    opencl_constant,
>    opencl_generic,
>  
>    cuda_device,
>    cuda_constant,
>    cuda_shared,
>  
>    Count,
> 
>    target_first = Count
>  };
>  
>  /// The type of a lookup table which maps from language-specific address spaces
>  /// to target-specific ones.
>  typedef unsigned Map[Count];
>  
>  }
> ```
> 
> Then this function would be:
> 
> ```
> unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
>   if (AS == LangAS::default && LangOpts.OpenCL)
>     // For OpenCL, only function local variables are not explicitly marked with an
>     // address space in the AST, and these need to be the address space of alloca.
>     return getTargetInfo().getDataLayout().getAllocaAddrSpace();
>   if (AS >= LangAS::target_first)
>     return AS - LangAS::target_first;
>   else
>     return (*AddrSpaceMap)[AS];
> }
> ```
> 
> Each target AddrSpaceMap would map LangAS::default to that target's default generic address space since that matches most other languages.
> 
> The address space attribute would need a corresponding "+ LangAS::target_first" to the value it stored in the AST.
> 
> Then it is possible to definitively tell when an AST node has not had any address space specified as it will be the LangAS::default value.
There is a lit test like this:

```
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only

#define OPENCL_CONSTANT 8388354
int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};

void foo() {
  c[0] = 1; //expected-error{{read-only variable is not assignable}}
}

```
It tries to set address space of opencl_constant through `__attribute__((address_space(n)))`. If we "+ LangAS::target_first" to the value before it is tored in the AST, we are not able to use `__attribute__((address_space(n)))` to represent opencl_constant.


https://reviews.llvm.org/D31404