[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

Fri Mar 31 10:09:48 PDT 2017

Anastasia added a comment.

In https://reviews.llvm.org/D31404#714906, @yaxunl wrote:

> In https://reviews.llvm.org/D31404#714244, @Anastasia wrote:
>
> > I can't see clearly why the alloca has to be extended to accommodate the address space too? Couldn't  the address space for alloca just be taken directly from the data layout?
> >
> > In fact is seems from the LLVM review, an address space for alloca doesn't go into the bitcode.
>
>
> In the latest comments of the LLVM review, reviewers have agreed that address space of alloca goes into the bitcode.
>
> I am not quite get your first question. Do you mean why the API of alloca has to have an address space parameter? Or do you question the necessity to let alloca returning a pointer pointing to non-zero address space?

I am asking mainly about the motivation of extending alloca instruction syntax with an extra address space. It seems to me that the address space for alloca could just be taken from the data layout without any modification to the instruction itself.

================
Comment at: include/clang/Basic/AddressSpaces.h:28
 enum ID {
-  Offset = 0x7FFF00,
+  Default = 0,

----------------
yaxunl wrote:
> Anastasia wrote:
> > Somehow I wish that opencl_private would be represented explicitly instead and then an absence of an address space attribute would signify the default one to be used. But since opencl_private has always been represented as an absence of an address space attribute not only in AST but in IR as well, I believe it might be a bigger change now. However, how does this default address space align with default AS we put during type parsing in processTypeAttrs (https://reviews.llvm.org/D13168). I think after this step we shouldn't need default AS explicitly any longer? 
> Currently in Clang having an address space qualifier value of 0 is equivalent to having no address space qualifier. This is due to the internal representation of address space qualifier as bits in a mask. Therefore although there are separate API's for hasAddressSpace() and getAddressSpace(), in many many places people just do not use hasAddressSpace() and only use getAddressSpace(). In a way, this is convenient, since it allows people to use just one unsigned to represent that whether a type has an address space qualifier and the value of the qualifier if it has one. That's why value 0 of address space qualifier is called `Default`, since it indicates `no address space qualifier`, which is the default situation. Here we give it the name `Default`, just to emphasise the existing reality, that is, 0 is truely the default value of address space qualifier. This also matches most languages' view of address space, that is, if not explicitly specified, 0 is the default address space qualifier since it means `no address space qualifier`.
> 
> For OpenCL 1.2, this matches perfectly to private address space, since if no address space qualifier implies private address space. For OpenCL 2.0, things become complicated. 'no address space qualifier' in the source code no longer ends up with a fixed address space qualifier in AST. What address space qualifier we get in AST depends on scope of the variable. To be consistent with the AST of OpenCL 1.2, we continue to use 'no address space qualifier (or 0 value address space qualifier)' in AST to represent private address space in OpenCL source language. This is non-ideal but it works fine. Therefore although it is not written, in fact opencl_private is 0.
> 
> Since address space 0 in AST always represents the private address space in OpenCL and the default address space in other languages, it cannot be used for other address spaces of OpenCL. Also, when mapped to target address space, for OpenCL, address space 0 in AST should map to target private address space or alloca address space; for other languages, address space 0 in AST should map to target generic address space. It would be clearer to have an enum value for 0 instead of using 0 directly.
I see a little value adding //Default// explicitly here, also because default AS is an OpenCL specific thing in my opinion. In C objects are just allowed to have no AS and it's fine. In fact the only use of //Default// in your patch is synonym to //opencl_private//. Unless we find another use for this, I would prefer not to add code for potential future use cases because it adds confusion and can be easily misused or create complications for adding new features.

I would rather say that Clang is missing an explicit representation of //opencl_private// at the moment. Mainly because private was used as default AS before OpenCL 2.0. So it was just the same thing. In OpenCL2.0 we worked around the fact that private is not represented explicitly by adding 'missing' ASes in //processTypeAttrs//. But it created some issues (for example in detecting //NULL// literal) which are still not solved.

Because ASes are represented as type qualifiers having //opencl_private// explicitly would align with C qualifiers concept (i.e. type with //const// would have a unique flag set to 1 in Qualifiers Mask and w/o //const// would have no flag set -> 0 ).

================
Comment at: include/clang/Basic/AddressSpaces.h:41
+
+  target_first = Count
 };
----------------
yaxunl wrote:
> Anastasia wrote:
> > I don't entirely understand the motivation for this. I think the idea of LangAS is to represent the source ASes while target ASes are reflected in the Map of Targets.cpp.
> There are two types of address spaces in languages end up as address spaces in AST:
> 
> 1. language defined address spaces, e.g. global in OpenCL => mapped to target address space
> 2. `__attribute__((address_space(n)))` => directly used as target address space with the same value
> 
> Since address space 0 in AST represents the default address space (no address space), it must be part of language address spaces and be mapped. Then it may be mapped to a target address space which is not 0.
> 
> Here is the problem: a user may use `__attribute__((address_space(0)))` to specify target address space 0, but he/she cannot, since address space 0 is always mapped as a language address space.
> 
> To solve this issue, address spaces from `__attribute__((address_space(n)))` is added to by Count when stored in AST. When mapped to target address space, their value is deducted by Count. Therefore, `__attribute__((address_space(0)))` becomes representable in AST.
I was mainly asking about adding //target_first//.

I think general intension of this enum was to specify all existing explicitly known ASes to be able to identify them easily in the code instead of using hard coded numbers. 

================
Comment at: lib/AST/ASTContext.cpp:9553
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+    return getTargetInfo().getDataLayout().getAllocaAddrSpace();
----------------
yaxunl wrote:
> Anastasia wrote:
> > Here it seems that LangAS::Default is indeed opencl_private?
> For OpenCL, that's true, however LangAS::Default may also be used by other languages to represent the default address space (i.e. no address space).
Do we know what the other use cases could be?

================
Comment at: test/Sema/invalid-assignment-constant-address-space.c:4
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};

----------------
yaxunl wrote:
> Anastasia wrote:
> > Is this test even correct? I don't think we can assume that C address spaces inherit the same restrictions as OpenCL. Especially that the notion of private/local/constant/global is an OpenCL specific thing.
> > 
> > I feel like Clang doesn't behave correctly for C address spaces now.
> > 
> > As for OpenCL I don't see why would anyone use __attribute__((address_space())) for constant AS. Especially that it's not part of the spec.
> I agree. There is no guarantee that in C language a user specified address space which happens to have the same address space value as OpenCL constant in AST will have the same semantics as OpenCL constant, because we only guarantee the semantics in OpenCL. For example, if we add a check for language for this diagnostic, this test will fail.
> 
> A user should not expect the same semantics. Only the target address space in the generated IR is guaranteed.
I think we should move this tests into CL and use __constant. Also it would be nice to add LangOpts.OpenCL check to where we give the error.

https://reviews.llvm.org/D31404