[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229
+    LangAS AddrSpaceR =
+        RHSType->getAs<BlockPointerType>()->getPointeeType().getAddressSpace();
+    CastKind Kind =
----------------
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > All of this can be much simpler:
> > > > > 
> > > > > ```
> > > > > LangAS AddrSpaceL = ToType->castAs<BlockPointerType>()->getPointeeType().getAddressSpace();
> > > > > LangAS AddrSpaceR = FromType->castAs<BlockPointerType>()->getPointeeType().getAddressSpace();
> > > > > ```
> > > > > 
> > > > > Is there something actually checking the validity of this address-space cast somewhere?
> > > > > Is there something actually checking the validity of this address-space cast somewhere?
> > > > 
> > > > The address spaces for blocks are currently added by clang implicitly. It doesn't seem possible to write kernel code qualifying blocks with address spaces. Although I have to say the error is not given properly because the parser gets confused at least for the examples I have tried. The OpenCL spec doesn't detail much regarding this use case. Potentially this is the area for improvement.
> > > > 
> > > > So for now we can add an assert to check the cast validity if you think it makes sense and maybe a FIXME in the  code to explain that address spaces aren't working with blocks....
> > > > The address spaces for blocks are currently added by clang implicitly. It doesn't seem possible to write kernel code qualifying blocks with address spaces.
> > > 
> > > There's no way that just fell out from the existing implementation; it was a change someone must have made for OpenCL.  Unless you care about blocks existing in multiple address spaces — which, given that you depend on eliminating them, I'm pretty sure you don't — the compiler just shouldn't do that if it's causing you problems.
> > So the reasons why we add addr spaces to blocks is that if they are declared in program scope they will be inferred as `__global` AS and if they are declared in kernel scope they are inferred as `__private` AS.
> > 
> > When there is a common code i.e. we pass block into a function or invoke the block we use generic AS so that blocks in different addr spaces can be work correctly but we are adding addr space cast.
> > 
> > This is the review that added this logic for OpenCL C: https://reviews.llvm.org/D28814
> > 
> > However in C++ we follow slightly different program path and therefore addr space cast isn't performed correctly.
> Okay, so users can't write block pointer types with explicit address spaces.  What exactly do you mean by "declaring" a block?  Do you mean that block pointer *types* are inferred to have different qualification based on where they're written, or do you mean that block *literals* have different qualification based on where they're written?  Because I'm not sure the latter is actually distinguishable from a language model in which blocks are always pointers into `__generic` and the compiler just immediately promotes them when emitting the expression.
We add `__generic` addr space to pointee type of block pointer type for all block variables. However, we don't do the same for block literals. Hence we need to generate conversion from `LangAS::Default` to `LangAS::opencl_generic`... I think this just aligns with what we do for other similar cases in OpenCL.

We also add `__global`/`__private` to block pointer type in block variable declarations, but we do the same for all other objects. At IR generation we generate block literals with captures as local variables in `__private` addr space and block literals without captures as global variables in `__global` addr space.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64083/new/

https://reviews.llvm.org/D64083