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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
