[PATCH] D36410: [OpenCL] Handle taking address of block captures

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.

There is an issue with taking an address of captured variables, because captures can be put in different locations depending on the vendor implementation (and therefore they are passed as generic AS pointer to the block).

The physical location for the captures can be:
(a) in case the block is called as a lambda function the captures are put on the stack - in the private AS.
(b) in case the block is used in enqueue it depends on the vendor implementation to put the captured content in the memory accessible for the enqueued kernels. In general case global memory can be used which is accessible everywhere.

Example:

  void foo(){
    int a;
    void (^bl)(void) = ^(void) {
      private int* b = &a; // a here is not the same object as a in foo
    }

Currently Clang hits `unreachable` due to `bitcast` of generic to private AS pointer because the original location of `a` is on the stack but the location of the captures is in generic AS.

This patch disallows taking address of captures because the physical address space can be different depending on the block use cases and vendor implementations.

An alternative approached could be (in case we want to allow this code) to assume captures are located in the generic AS implicitly. However, in this case programmers should be advised that erroneous AS casts can occur further that can't be diagnosed by the frontend (i.e. if capture address is used further in the operations of a different address space to where the captures physical location is).

Example:

  void foo(){
    int a;
    void (^bl)(void) = ^(void) {
      local int* b = (local int*)&a; //valid cast of implicitly generic to local but &a will be located in private or global AS for the 2 uses below
    }
    b();
    enqueue_kernel(..., b)
  }

@Sam, @Alexey, does it make sense taking this route?


https://reviews.llvm.org/D36410

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/SemaOpenCL/invalid-block.cl


Index: test/SemaOpenCL/invalid-block.cl
===================================================================
--- test/SemaOpenCL/invalid-block.cl
+++ test/SemaOpenCL/invalid-block.cl
@@ -78,3 +78,14 @@
   };
   return;
 }
+
+// Taking address of a capture is not allowed
+int g;
+kernel void f7(int a1) {
+  int a2;
+  void (^bl)(void) = ^(void) {
+    &g; //expected-warning{{expression result unused}}
+    &a1; //expected-error{{taking address of a capture is not allowed}}
+    &a2; //expected-error{{taking address of a capture is not allowed}}
+  };
+}
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -526,14 +526,6 @@
   assert(!Ty.isNull() && "DefaultFunctionArrayConversion - missing type");
 
   if (Ty->isFunctionType()) {
-    // If we are here, we are not calling a function but taking
-    // its address (which is not allowed in OpenCL v1.0 s6.8.a.3).
-    if (getLangOpts().OpenCL) {
-      if (Diagnose)
-        Diag(E->getExprLoc(), diag::err_opencl_taking_function_address);
-      return ExprError();
-    }
-
     if (auto *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
       if (auto *FD = dyn_cast<FunctionDecl>(DRE->getDecl()))
         if (!checkAddressOfFunctionIsAvailable(FD, Diagnose, E->getExprLoc()))
@@ -10690,10 +10682,17 @@
   // Make sure to ignore parentheses in subsequent checks
   Expr *op = OrigOp.get()->IgnoreParens();
 
-  // OpenCL v1.0 s6.8.a.3: Pointers to functions are not allowed.
-  if (LangOpts.OpenCL && op->getType()->isFunctionType()) {
-    Diag(op->getExprLoc(), diag::err_opencl_taking_function_address);
-    return QualType();
+  // In OpenCL captures for blocks called as lambda functions
+  // are located in the private address space. Blocks used in
+  // enqueue_kernel can be located in a different address space
+  // depending on a vendor implementation. Thus preventing
+  // taking an address of the capture to avoid invalid AS casts.
+  if (LangOpts.OpenCL) {
+    auto* VarRef = dyn_cast<DeclRefExpr>(op);
+    if (VarRef && VarRef->refersToEnclosingVariableOrCapture()) {
+      Diag(op->getExprLoc(), diag::err_opencl_taking_address_capture);
+      return QualType();
+    }
   }
 
   if (getLangOpts().C99) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -6948,8 +6948,8 @@
 def err_opencl_function_pointer_variable : Error<
   "pointers to functions are not allowed">;
 
-def err_opencl_taking_function_address : Error<
-  "taking address of function is not allowed">;
+def err_opencl_taking_address_capture : Error<
+  "taking address of a capture is not allowed">;
 
 def err_invalid_conversion_between_vector_and_scalar : Error<
   "invalid conversion between vector type %0 and scalar type %1">;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D36410.110015.patch
Type: text/x-patch
Size: 2955 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170807/79cdacbb/attachment-0001.bin>