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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
