[OpenCL][Sema] Improve address space support for blocks

[mantognini]

Summary

This patch ensures that the following code is compiled identically with
-cl-std=CL2.0 and -fblocks -cl-std=c++.

kernel void test(void) {
  void (^const block_A)(void) = ^{
    return;
  };
}

A new test is not added because cl20-device-side-enqueue.cl will cover
this once blocks are further improved for C++ for OpenCL.

The changes to Sema::PerformImplicitConversion are based on
the parts of Sema::CheckAssignmentConstraints on block pointer
conversions.

[Anastasia]

LGTM! Thanks! I would suggest to wait a day or two before committing just in case John has any feedback.

[rjmccall]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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?

[Anastasia]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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....

[rjmccall]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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.

[Anastasia]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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.

[rjmccall]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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.

[Anastasia]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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.

[mantognini]

cfe/trunk/lib/Sema/SemaExprCXX.cpp

4229

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.

4229

I'll keep this in mind and provide a patch soonish for the code simplification.

[rjmccall]

A qualifier "outside" the block-pointer type is telling you what address space the object which holds the block pointer is in, which is a completely different thing. Instead of __global int (^block4)(void) = ^{ return 0; };, you need to write int (^__global block4)(void) = ^{ return 0; };.

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.

Okay, so it sounds like *from the language perspective* all block pointers are actually pointers into __generic, and the thing with literals is just an implementation detail that's been inappropriately expressed in the AST. The better implementation strategy is to make sure that (1) the AST uses the size/alignment of a __generic pointer for a block pointer and (2) IRGen implicitly converts block literals to __generic pointers when it emits them, and then there's no such thing as a block pointer to a qualified type.

[rjmccall]

In D64083#1583109, @rjmccall wrote:

Okay, so it sounds like *from the language perspective* all block pointers are actually pointers into __generic, and the thing with literals is just an implementation detail that's been inappropriately expressed in the AST. The better implementation strategy is to make sure that (1) the AST uses the size/alignment of a __generic pointer for a block pointer and (2) IRGen implicitly converts block literals to __generic pointers when it emits them, and then there's no such thing as a block pointer to a qualified type.

This is actually important to get right in C++ because of auto and templates; you really don't want non-generic block pointer types to become an expressible thing in the language.

[Anastasia]

In D64083#1583110, @rjmccall wrote:

In D64083#1583109, @rjmccall wrote:

Okay, so it sounds like *from the language perspective* all block pointers are actually pointers into __generic, and the thing with literals is just an implementation detail that's been inappropriately expressed in the AST. The better implementation strategy is to make sure that (1) the AST uses the size/alignment of a __generic pointer for a block pointer and (2) IRGen implicitly converts block literals to __generic pointers when it emits them, and then there's no such thing as a block pointer to a qualified type.

This is actually important to get right in C++ because of auto and templates; you really don't want non-generic block pointer types to become an expressible thing in the language.

Yes, I agree. Right now it fails but the diagnostic is wrong.

In D64083#1583110, @rjmccall wrote:

In D64083#1583109, @rjmccall wrote:

Okay, so it sounds like *from the language perspective* all block pointers are actually pointers into __generic, and the thing with literals is just an implementation detail that's been inappropriately expressed in the AST. The better implementation strategy is to make sure that (1) the AST uses the size/alignment of a __generic pointer for a block pointer and (2) IRGen implicitly converts block literals to __generic pointers when it emits them, and then there's no such thing as a block pointer to a qualified type.

This is actually important to get right in C++ because of auto and templates; you really don't want non-generic block pointer types to become an expressible thing in the language.

I agree. I have opened a bug to follow up on this: https://bugs.llvm.org/show_bug.cgi?id=42621. We can still simplify the code in this patch for now, although I expect in the future it won't be needed.

[rjmccall]

Sounds good.

cfe/trunk/lib/Sema/SemaExprCXX.cpp

// Case 2.  _Complex x -> y
         From = ImpCastExprToType(From, ToType,
                    isFloatingComplex ? CK_FloatingToIntegral : CK_IntegralCast,
                                  VK_RValue, /*BasePath=*/nullptr, CCK).get();
       }
     }
     break;
 
   case ICK_Block_Pointer_Conversion: {
-    From = ImpCastExprToType(From, ToType.getUnqualifiedType(), CK_BitCast,
+    QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
+    QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
+
+    // Assumptions based on Sema::IsBlockPointerConversion.
+    assert(isa<BlockPointerType>(LHSType) && "BlockPointerType expected");
+    assert(isa<BlockPointerType>(RHSType) && "BlockPointerType expected");
+
+    LangAS AddrSpaceL =
+        LHSType->getAs<BlockPointerType>()->getPointeeType().getAddressSpace();
+    LangAS AddrSpaceR =
+        RHSType->getAs<BlockPointerType>()->getPointeeType().getAddressSpace();
+    CastKind Kind =
+        AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
+    From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
                              VK_RValue, /*BasePath=*/nullptr, CCK).get();
     break;
   }
 
   case ICK_TransparentUnionConversion: {
     ExprResult FromRes = From;
     Sema::AssignConvertType ConvTy =
       CheckTransparentUnionArgumentConstraints(ToType, FromRes);