t-tye added inline comments.
================ Comment at: lib/AST/ASTContext.cpp:9547-9555 +unsigned ASTContext::getTargetAddressSpace(unsigned AS) const { + // For OpenCL, the address space qualifier is 0 in AST. + if (AS == 0 && LangOpts.OpenCL) + return getTargetInfo().getDataLayout().getAllocaAddrSpace(); + if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count) + return AS; + else ---------------- yaxunl wrote: > t-tye wrote: > > yaxunl wrote: > > > t-tye wrote: > > > > Presumably this will not work if the user put an address space > > > > attribute on a variable with an address space of 0, since it is not > > > > possible to distinguish between a variable that never had an attribute > > > > and was function local, and one that has an explicit address space > > > > attribute specifying 0. > > > > > > > > It would seem better if LangAS did not map the 0..LangAS::Offset to be > > > > target address spaces. Instead LangAS could use 0..LangAS::Count to be > > > > the CLANG explicitly specified values (reserving 0 to mean the default > > > > when none was specified), and values above LangAS::Count would map to > > > > the explicitly specified target address spaces. For example: > > > > > > > > ``` > > > > namespace clang { > > > > > > > > namespace LangAS { > > > > > > > > /// \brief Defines the set of possible language-specific address > > > > spaces. > > > > /// > > > > /// This uses values above the language-specific address spaces to > > > > denote > > > > /// the target-specific address spaces biased by target_first. > > > > enum ID { > > > > default = 0, > > > > > > > > opencl_global, > > > > opencl_local, > > > > opencl_constant, > > > > opencl_generic, > > > > > > > > cuda_device, > > > > cuda_constant, > > > > cuda_shared, > > > > > > > > Count, > > > > > > > > target_first = Count > > > > }; > > > > > > > > /// The type of a lookup table which maps from language-specific > > > > address spaces > > > > /// to target-specific ones. > > > > typedef unsigned Map[Count]; > > > > > > > > } > > > > ``` > > > > > > > > Then this function would be: > > > > > > > > ``` > > > > unsigned ASTContext::getTargetAddressSpace(unsigned AS) const { > > > > if (AS == LangAS::default && LangOpts.OpenCL) > > > > // For OpenCL, only function local variables are not explicitly > > > > marked with an > > > > // address space in the AST, and these need to be the address space > > > > of alloca. > > > > return getTargetInfo().getDataLayout().getAllocaAddrSpace(); > > > > if (AS >= LangAS::target_first) > > > > return AS - LangAS::target_first; > > > > else > > > > return (*AddrSpaceMap)[AS]; > > > > } > > > > ``` > > > > > > > > Each target AddrSpaceMap would map LangAS::default to that target's > > > > default generic address space since that matches most other languages. > > > > > > > > The address space attribute would need a corresponding "+ > > > > LangAS::target_first" to the value it stored in the AST. > > > > > > > > Then it is possible to definitively tell when an AST node has not had > > > > any address space specified as it will be the LangAS::default value. > > > There is a lit test like this: > > > > > > ``` > > > // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only > > > > > > #define OPENCL_CONSTANT 8388354 > > > int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0}; > > > > > > void foo() { > > > c[0] = 1; //expected-error{{read-only variable is not assignable}} > > > } > > > > > > ``` > > > It tries to set address space of opencl_constant through > > > `__attribute__((address_space(n)))`. If we "+ LangAS::target_first" to > > > the value before it is tored in the AST, we are not able to use > > > `__attribute__((address_space(n)))` to represent opencl_constant. > > This seems a bit of a hack. It could be made to work by simply defining > > OPENCL_CONSTANT to be the negative value that would result in the correct > > LangAS value, which is pretty much what the test is doing anyway. Just > > seems conflating the default value with the first target address space > > value is undesirable as it prevents specifying target address space 0 as > > that gets treated differently than any other address space value. > Clang will emit error if address space value is negative, but I can change it > to a warning. I guess the observation is that without a change the proposed changes will make __attribute__((address_space(0))) behave in unexpected ways for some targets (for AMDGPU it would actually cause address space for private to be used). The suggested approach also seems cleaner as it explicitly defines a "default" value which does not overlay the target address space range of values. https://reviews.llvm.org/D31404 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits