Anastasia added inline comments.
================ Comment at: include/clang/AST/ASTContext.h:2328 + return AddrSpaceMapMangling || + AS >= LangAS::target_first; } ---------------- yaxunl wrote: > 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. > > > 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. In your patch the default AS corresponds to value set in the target address space map and not taken from the alloca AS of the DataLayout. ================ Comment at: include/clang/Basic/AddressSpaces.h:32 + // QualType represents private address space in OpenCL source code. + Default = 0, + ---------------- The alloca AS is not taken from the target AS map but from the DataLayout. This keep me wonder whether the explicit Default item is actually needed here.... ================ Comment at: lib/Basic/Targets.cpp:8382 static const unsigned SPIRAddrSpaceMap[] = { + 4, // Default 1, // opencl_global ---------------- This will break use case of compiling for SPIR from C which is needed by some frameworks. We can only use generic if compiled in OpenCL2.0 mode and only for pointer types. ================ Comment at: lib/Sema/SemaExprCXX.cpp:3121 - if (unsigned AddressSpace = Pointee.getAddressSpace()) + if (Pointee.getQualifiers().getAddressSpace()) return Diag(Ex.get()->getLocStart(), ---------------- yaxunl wrote: > Anastasia wrote: > > `Pointee.getAddressSpace()` wouldn't work any more? > It still works. I will fix it. Do you plan to update this? ================ Comment at: test/CodeGen/default-address-space.c:51 +// CHECK-LABEL: define void @test4(i32* %a) +// CHECK: %[[a_addr:.*]] = alloca i32*, align 4, addrspace(5) +// CHECK: store i32* %a, i32* addrspace(5)* %[[a_addr]] ---------------- The default AS is not the same as alloca AS then? https://reviews.llvm.org/D31404 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
