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

Yaxun Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 11:58:51 PDT 2017


yaxunl marked 8 inline comments as done.
yaxunl added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:2328
+    return AddrSpaceMapMangling || 
+           AS >= LangAS::target_first;
   }
----------------
Anastasia wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > So we couldn't use the  LangAS::Count instead?
> > > > 
> > > > I have the same comment in other places that use LangAS::target_first. Why couldn't we simply use LangAS::Count? It there any point in having two tags?
> > > > 
> > > > Another comment is why do we need ASes specified by `__attribute__((address_space(n)))` to be unique enum number at the end of named ASes of OpenCL and CUDA? I think conceptually the full range of ASes can be used in C because the ASes from OpenCL and CUDA are not available there anyways.
> > > I will use LangAS::Count instead and remove target_first, since their values are the same.
> > > 
> > > For your second question:  the values for `__attribute__((address_space(n)))` need to be different from the language specific address space values because they are mapped to target address space differently.
> > > 
> > > For language specific address space, they are mapped through a target defined mapping table.
> > > 
> > > For `__attribute__((address_space(n)))`, the target address space should be the same as n, without going through the mapping table.
> > > 
> > > If they are defined in overlapping value ranges, they cannot be handled in different ways.
> > > 
> > > 
> > Target address space map currently corresponds to the named address spaces of OpenCL and CUDA only. So if the idea is to avoid overlapping with those we should extend the table? Although, I don't see how this can be done because it will require fixed semantic of address spaces in C which is currently undefined.
> Perhaps I am missing something but I don't see any change here that makes non-named address spaces follow different path for the target.
> 
> Also does this change relate to alloca extension in some way? I still struggle to understand this fully...
> 
> All I can see is that this change restricts overlapping of named and non-named address spaces but I can't clearly understand the motivation for this.
`__attribute__((address_space(n)))` is used in C and C++ to specify target address space for a variable. 

For example, https://github.com/llvm-mirror/clang/blob/master/test/Sema/address_spaces.c 

Many cases they just need to put a variable in certain target address space and do not need specific semantics for these address spaces.


================
Comment at: include/clang/AST/ASTContext.h:2328
+    return AddrSpaceMapMangling || 
+           AS >= LangAS::target_first;
   }
----------------
yaxunl wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > So we couldn't use the  LangAS::Count instead?
> > > > > 
> > > > > I have the same comment in other places that use LangAS::target_first. Why couldn't we simply use LangAS::Count? It there any point in having two tags?
> > > > > 
> > > > > Another comment is why do we need ASes specified by `__attribute__((address_space(n)))` to be unique enum number at the end of named ASes of OpenCL and CUDA? I think conceptually the full range of ASes can be used in C because the ASes from OpenCL and CUDA are not available there anyways.
> > > > I will use LangAS::Count instead and remove target_first, since their values are the same.
> > > > 
> > > > For your second question:  the values for `__attribute__((address_space(n)))` need to be different from the language specific address space values because they are mapped to target address space differently.
> > > > 
> > > > For language specific address space, they are mapped through a target defined mapping table.
> > > > 
> > > > For `__attribute__((address_space(n)))`, the target address space should be the same as n, without going through the mapping table.
> > > > 
> > > > If they are defined in overlapping value ranges, they cannot be handled in different ways.
> > > > 
> > > > 
> > > Target address space map currently corresponds to the named address spaces of OpenCL and CUDA only. So if the idea is to avoid overlapping with those we should extend the table? Although, I don't see how this can be done because it will require fixed semantic of address spaces in C which is currently undefined.
> > Perhaps I am missing something but I don't see any change here that makes non-named address spaces follow different path for the target.
> > 
> > Also does this change relate to alloca extension in some way? I still struggle to understand this fully...
> > 
> > All I can see is that this change restricts overlapping of named and non-named address spaces but I can't clearly understand the motivation for this.
> `__attribute__((address_space(n)))` is used in C and C++ to specify target address space for a variable. 
> 
> For example, https://github.com/llvm-mirror/clang/blob/master/test/Sema/address_spaces.c 
> 
> Many cases they just need to put a variable in certain target address space and do not need specific semantics for these address spaces.
In the definition of `ASTContext::getTargetAddressSpace()` you can see how address space values below and above LangAS::Count are handled differently.

The old definition of address space enum does not differentiate between the default address space 0 (no address space qualifier) and `__attribute__((address_space(0)))`.

Before alloca API changes, this does not matter, since address space 0 in AST is always mapped to target address space 0.

However, after alloca API changes, default address space 0 is mapped to target alloca address space, which may be non-zero. Therefore it is necessary to differentiate between `__attribute__((address_space(0)))` and the default address space 0. Otherwise, if a user specifies `__attribute__((address_space(0)))`, it may not be mapped to target address space 0.




================
Comment at: lib/Basic/Targets.cpp:2035
 static const LangAS::Map AMDGPUPrivateIsZeroMap = {
+    4,  // Default
     1,  // opencl_global
----------------
Anastasia wrote:
> This should use address space attribute 4 for all non-AS type objects. Could we make sure we have a codegen test for this? 
Will do.


================
Comment at: lib/Sema/SemaExprCXX.cpp:3121
 
-    if (unsigned AddressSpace = Pointee.getAddressSpace())
+    if (Pointee.getQualifiers().getAddressSpace())
       return Diag(Ex.get()->getLocStart(),
----------------
Anastasia wrote:
> `Pointee.getAddressSpace()` wouldn't work any more?
It still works. I will fix it.


https://reviews.llvm.org/D31404





More information about the cfe-commits mailing list