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

[Tony Tye via Phabricator via cfe-commits]

t-tye 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
----------------
yaxunl wrote:
> 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.
This seems a bit of a hack. It could be made to work by simply defining OPENCL_CONSTANT to be the negative value that would result in the correct LangAS value, which is pretty much what the test is doing anyway. Just seems conflating the default value with the first target address space value is undesirable as it prevents specifying target address space 0 as that gets treated differently than any other address space value.


https://reviews.llvm.org/D31404