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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to