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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 10:45:10 PDT 2017


Anastasia added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:2328
+    return AddrSpaceMapMangling || 
+           AS >= LangAS::target_first;
   }
----------------
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.


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


https://reviews.llvm.org/D31404





More information about the cfe-commits mailing list