[SYCL] Disallow explicit casts between mismatching address spaces

[bader]

LGTM. Thanks!

[hvdijk]

Hi, what is the rationale here? This reuses the logic that was written for OpenCL mode, but in OpenCL mode, it was made an error with the idea that a new keyword addrspace_cast could be used in those cases where the user actually wants an address space cast. Here, in SYCL mode, it's just made an error with no way out for the user that I can see if they actually want this. Is this really correct?

Edit: apologies, I misread what was done with addrspace_cast, that new operator only allows conversions that are otherwise also allowed. Based on that, this change does actually align with what was done for OpenCL mode, it does not restrict anything that is allowed in OpenCL mode. It does make sense, then. A slightly more verbose commit message might have helped though :)

[keryell]

In D118935#3389452, @hvdijk wrote:

It does make sense, then. A slightly more verbose commit message might have helped though :)

Even better, some comments in the code explaining the "why" would have helped. The commit messages disappear while the comments are right there when the code is read/reviewed.

[Fznamznon]

I misread what was done with addrspace_cast, that new operator only allows conversions that are otherwise also allowed. Based on that, this change does actually align with what was done for OpenCL mode, it does not restrict anything that is allowed in OpenCL mode. It does make sense, then. A slightly more verbose commit message might have helped though :)

Right, the intention of this change is to re-use OpenCL mode logic. There is no connection between addrspace_cast operator and this change. In fact, there is no such operator in SYCL.

Even better, some comments in the code explaining the "why" would have helped.

I was under impression that the change is small and therefore easy to understand. Would some comment like "SYCL re-uses OpenCL mode diagnostics to emit errors in case the cast happens between disjoint address spaces" help?

[keryell]

Yes, your comment idea looks good and helpful to me.
Thanks.

[hvdijk]

In D118935#3399095, @Fznamznon wrote:

I was under impression that the change is small and therefore easy to understand. Would some comment like "SYCL re-uses OpenCL mode diagnostics to emit errors in case the cast happens between disjoint address spaces" help?

Speaking only for myself, but it was clear to me that that's what it did. What wasn't clear to me was why that's okay, and that was because I was under the impression that there were ways around this in OpenCL that are unavailable in SYCL. What might have helped me was a comment not saying that the cast is also an error is OpenCL mode, but that there is no way at all to express the conversion in OpenCL mode. This is all in hindsight though, I am not sure to what extent my confusion was foreseeable.

clang/lib/Sema/SemaCast.cpp

static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
   // So we finish by allowing everything that remains - it's got to be two
   // object pointers.
   return SuccessResult;
 }
 
 static TryCastResult TryAddressSpaceCast(Sema &Self, ExprResult &SrcExpr,
                                          QualType DestType, bool CStyle,
                                          unsigned &msg, CastKind &Kind) {
-  if (!Self.getLangOpts().OpenCL)
+  if (!Self.getLangOpts().OpenCL && !Self.getLangOpts().SYCLIsDevice)
     // FIXME: As compiler doesn't have any information about overlapping addr
     // spaces at the moment we have to be permissive here.
     return TC_NotApplicable;
   // Even though the logic below is general enough and can be applied to
   // non-OpenCL mode too, we fast-path above because no other languages
   // define overlapping address spaces currently.
   auto SrcType = SrcExpr.get()->getType();
   // FIXME: Should this be generalized to references? The reference parameter

clang/test/CodeGenSYCL/address-space-conversions.cpp

void usages() {
   // CHECK-DAG: [[PRIV]].ascast = addrspacecast i32** [[PRIV]] to i32* addrspace(4)*
   __attribute__((opencl_private)) int *PRIV;
   // CHECK-DAG: [[GLOB_DEVICE:%[a-zA-Z0-9]+]] = alloca i32 addrspace(5)*
   __attribute__((opencl_global_device)) int *GLOBDEVICE;
   // CHECK-DAG: [[GLOB_HOST:%[a-zA-Z0-9]+]] = alloca i32 addrspace(6)*
   __attribute__((opencl_global_host)) int *GLOBHOST;
 
   // Explicit conversions
-  // From names address spaces to default address space
+  // From named address spaces to default address space
   // CHECK-DAG: [[GLOB_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(1)*, i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
   // CHECK-DAG: [[GLOB_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(1)* [[GLOB_LOAD]] to i32 addrspace(4)*
   // CHECK-DAG: store i32 addrspace(4)* [[GLOB_CAST]], i32 addrspace(4)* addrspace(4)* [[NoAS]].ascast
   NoAS = (int *)GLOB;
   // CHECK-DAG: [[LOC_LOAD:%[a-zA-Z0-9]+]] = load i32 addrspace(3)*, i32 addrspace(3)* addrspace(4)* [[LOC]].ascast
   // CHECK-DAG: [[LOC_CAST:%[a-zA-Z0-9]+]] = addrspacecast i32 addrspace(3)* [[LOC_LOAD]] to i32 addrspace(4)*
   // CHECK-DAG: store i32 addrspace(4)* [[LOC_CAST]], i32 addrspace(4)* addrspace(4)* [[NoAS]].ascast
   NoAS = (int *)LOC;

clang/test/SemaSYCL/address-space-conversions.cpp

void usages() {
   tmpl(PRIV);
   tmpl(NoAS);
   tmpl(LOC);
 
   // Implicit casts to named address space are disallowed
   baz(NoAS);                                   // expected-error {{no matching function for call to 'baz'}}
   __attribute__((opencl_local)) int *l = NoAS; // expected-error {{cannot initialize a variable of type '__local int *' with an lvalue of type 'int *'}}
 
+  // Explicit casts between disjoint address spaces are disallowed
+  GLOB = (__attribute__((opencl_global)) int *)PRIV; // expected-error {{C-style cast from '__private int *' to '__global int *' converts between mismatching address spaces}}
+
   (void)static_cast<int *>(GLOB);
   (void)static_cast<void *>(GLOB);
   int *i = GLOB;
   void *v = GLOB;
   (void)i;
   (void)v;
 
   __attribute__((opencl_global_host)) int *GLOB_HOST;