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
