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

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked 2 inline comments as done.
mantognini added inline comments.


================
Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229
+    LangAS AddrSpaceR =
+        RHSType->getAs<BlockPointerType>()->getPointeeType().getAddressSpace();
+    CastKind Kind =
----------------
Anastasia wrote:
> 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.
I've been experimenting a bit more with blocks. Here is a snippet that helped me understand things further:

```
typedef int (^block_ty)(void);
__global block_ty block3 = ^{ return 0; };
__global int (^block4)(void) = ^{ return 0; }; // FIXME return type is not allowed to have AS qualifier

kernel void test(void) {
  block_ty __constant block0 = ^{ return 0; };
  int (^block1)(void) = ^(void){ return 0; };
  __private block_ty block2 = ^{ return 0; };
  // block2 = ^{ return 1; }; // invalid code in OpenCL; blocks cannot be re-assigned.

  int x = ((__local block_ty) block3)(); // FIXME The AS cast is missing from the AST, plus Clang crashes.
}
```

This piece of code shows two bugs:
* Address space qualifier on the return type of blocks should be rejected.
* Address space cast are missing from the AST when doing a C cast, and this regardless of the `-cl-std` mode.

I'll open bugs for those.

Aside those issues, it seems that:
- All block //expressions// in this program are typed as `int (^)(void)` with default address space;
- The //type// of the block variables (VarDecl) can be qualified with an address space;
- The user cannot change/specify the address space of the blocks themeselves -- The OpenCL specification doesn't say a thing about AS and Blocks in fact.
- The only way I've found to change the AS of a block variable is by using a typedef.


================
Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229
+    LangAS AddrSpaceR =
+        RHSType->getAs<BlockPointerType>()->getPointeeType().getAddressSpace();
+    CastKind Kind =
----------------
mantognini wrote:
> Anastasia wrote:
> > 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.
> I've been experimenting a bit more with blocks. Here is a snippet that helped me understand things further:
> 
> ```
> typedef int (^block_ty)(void);
> __global block_ty block3 = ^{ return 0; };
> __global int (^block4)(void) = ^{ return 0; }; // FIXME return type is not allowed to have AS qualifier
> 
> kernel void test(void) {
>   block_ty __constant block0 = ^{ return 0; };
>   int (^block1)(void) = ^(void){ return 0; };
>   __private block_ty block2 = ^{ return 0; };
>   // block2 = ^{ return 1; }; // invalid code in OpenCL; blocks cannot be re-assigned.
> 
>   int x = ((__local block_ty) block3)(); // FIXME The AS cast is missing from the AST, plus Clang crashes.
> }
> ```
> 
> This piece of code shows two bugs:
> * Address space qualifier on the return type of blocks should be rejected.
> * Address space cast are missing from the AST when doing a C cast, and this regardless of the `-cl-std` mode.
> 
> I'll open bugs for those.
> 
> Aside those issues, it seems that:
> - All block //expressions// in this program are typed as `int (^)(void)` with default address space;
> - The //type// of the block variables (VarDecl) can be qualified with an address space;
> - The user cannot change/specify the address space of the blocks themeselves -- The OpenCL specification doesn't say a thing about AS and Blocks in fact.
> - The only way I've found to change the AS of a block variable is by using a typedef.
I'll keep this in mind and provide a patch soonish for the code simplification.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64083