[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.
Herald added a project: All.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Mariya Podchishchaeva via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1cee9608982a: [SYCL] Disallow explicit casts between 
mismatching address spaces (authored by Fznamznon).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

Files:
  clang/lib/Sema/SemaCast.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp


Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -55,6 +55,9 @@
   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(GLOB);
   (void)static_cast(GLOB);
   int *i = GLOB;
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -35,7 +35,7 @@
   __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
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -2545,7 +2545,7 @@
 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;


Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -55,6 +55,9 @@
   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(GLOB);
   (void)static_cast(GLOB);
   int *i = GLOB;
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -35,7 +35,7 @@
   __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
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -2545,7 +2545,7 @@
 static TryCastResult TryAddressSpaceCast(Sema &Self, ExprResult &SrcExpr,
  QualType DestType, bool CStyle,
  unsigned &msg, CastKind &Kind) {
-  if (!Self.getLangOpts().OpenCL)
+  if (!Self.getLangOpts().OpenCL && !Self.getLangOp

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon created this revision.
Fznamznon added a reviewer: bader.
Herald added subscribers: Naghasan, Anastasia, ebevhan, yaxunl.
Fznamznon requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118935

Files:
  clang/lib/Sema/SemaCast.cpp
  clang/test/CodeGenSYCL/address-space-conversions.cpp
  clang/test/SemaSYCL/address-space-conversions.cpp


Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -55,6 +55,9 @@
   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(GLOB);
   (void)static_cast(GLOB);
   int *i = GLOB;
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -35,7 +35,7 @@
   __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
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -2545,7 +2545,7 @@
 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;


Index: clang/test/SemaSYCL/address-space-conversions.cpp
===
--- clang/test/SemaSYCL/address-space-conversions.cpp
+++ clang/test/SemaSYCL/address-space-conversions.cpp
@@ -55,6 +55,9 @@
   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(GLOB);
   (void)static_cast(GLOB);
   int *i = GLOB;
Index: clang/test/CodeGenSYCL/address-space-conversions.cpp
===
--- clang/test/CodeGenSYCL/address-space-conversions.cpp
+++ clang/test/CodeGenSYCL/address-space-conversions.cpp
@@ -35,7 +35,7 @@
   __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
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -2545,7 +2545,7 @@
 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