[PATCH] D58236: Make address space conversions a bit stricter.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360258: [Sema][OpenCL] Make address space conversions a bit 
stricter. (authored by stulova, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58236?vs=195520=198657#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58236

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaCast.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/CodeGenOpenCL/numbered-address-space.cl
  cfe/trunk/test/SemaOpenCL/address-spaces.cl
  cfe/trunk/test/SemaOpenCL/event_t_overload.cl
  cfe/trunk/test/SemaOpenCL/numbered-address-space.cl
  cfe/trunk/test/SemaOpenCL/queue_t_overload.cl

Index: cfe/trunk/test/SemaOpenCL/numbered-address-space.cl
===
--- cfe/trunk/test/SemaOpenCL/numbered-address-space.cl
+++ cfe/trunk/test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: cfe/trunk/test/SemaOpenCL/queue_t_overload.cl
===
--- cfe/trunk/test/SemaOpenCL/queue_t_overload.cl
+++ cfe/trunk/test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: cfe/trunk/test/SemaOpenCL/address-spaces.cl
===
--- cfe/trunk/test/SemaOpenCL/address-spaces.cl
+++ cfe/trunk/test/SemaOpenCL/address-spaces.cl
@@ -124,6 +124,106 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of nested pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes address space of pointer}}
+
+  ll = g;// expected-error {{assigning '__global int *' to '__local int **' changes address space of pointer}}
+  ll = gg;   // expected-error {{assigning '__global int **' to '__local int **' changes address space of nested pointer}}
+  ll = l;// expected-error {{assigning '__local int *' to '__local int **' changes address space of pointer}}
+  ll = gg_f; // expected-error {{assigning '__global float **' to '__local int **' changes address space of nested pointer}}
+  ll = (__local int * __private *)gg_f; // expected-warning {{casting '__global float **' to type 

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D58236#1494722 , @ebevhan wrote:

> This was accepted a while ago, but never landed. I don't have commit access; 
> could someone commit it?


Sure!


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

This was accepted a while ago, but never landed. I don't have commit access; 
could someone commit it?


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Alright, this seems reasonable to me.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 195520.
ebevhan edited the summary of this revision.

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

https://reviews.llvm.org/D58236

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/numbered-address-space.cl
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/event_t_overload.cl
  test/SemaOpenCL/numbered-address-space.cl
  test/SemaOpenCL/queue_t_overload.cl

Index: test/SemaOpenCL/queue_t_overload.cl
===
--- test/SemaOpenCL/queue_t_overload.cl
+++ test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- test/SemaOpenCL/numbered-address-space.cl
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -7,5 +7,5 @@
   event_t evt;
   foo(evt, src1);
   foo(0, src2);
-  foo(evt, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(evt, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -124,6 +124,106 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of nested pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes address space of pointer}}
+
+  ll = g;// expected-error {{assigning '__global int *' to '__local int **' changes address space of pointer}}
+  ll = gg;   // expected-error {{assigning '__global int **' to '__local int **' changes address space of nested pointer}}
+  ll = l;// expected-error {{assigning '__local int *' to '__local int **' changes address space of pointer}}
+  ll = gg_f; // expected-error {{assigning '__global float **' to '__local int **' changes address space of nested pointer}}
+  ll = (__local int * __private *)gg_f; // expected-warning {{casting '__global float **' to type '__local int **' discards qualifiers in nested pointer types}}

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D58236#1467151 , @Anastasia wrote:

> In D58236#1466263 , @ebevhan wrote:
>
> > Well, it doesn't seem to me like there is consensus on prohibiting nested 
> > address space conversion like this.
> >
> > I can simply redo the patch to only include the bugfix on implicit 
> > conversions and drop the nesting level checks.
>
>
> I thought the conclusion is:
>
> - Explicit conversions allowed for nested pointers with a warning.
> - Implicit conversions are disallowed for nested pointers with an error.
>
>   Do you not see it this way?


Alright, that seems reasonable to me. Some of the patch goes away, then. I'll 
update it.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D58236#1466263 , @ebevhan wrote:

> Well, it doesn't seem to me like there is consensus on prohibiting nested 
> address space conversion like this.
>
> I can simply redo the patch to only include the bugfix on implicit 
> conversions and drop the nesting level checks.


I thought the conclusion is:

- Explicit conversions allowed for nested pointers with a warning.
- Implicit conversions are disallowed for nested pointers with an error.

Do you not see it this way?


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Well, it doesn't seem to me like there is consensus on prohibiting nested 
address space conversion like this.

I can simply redo the patch to only include the bugfix on implicit conversions 
and drop the nesting level checks.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D58236#1443556 , @ebevhan wrote:

> In D58236#1443082 , @rjmccall wrote:
>
> > I think C probably requires us to allow this under an explicit cast, but we 
> > can at least warn about it.  It does not require us to allow this as an 
> > implicit conversion, and that should be an error.
>
>
> Are you referring to casts to and from `void*`? I think the standard says 
> those have to be allowed, and I don't see anything about whether or not the 
> conversion has to be explicit.


No, I mean casts between pointer types that change the pointee address space.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D58236#1443082 , @rjmccall wrote:

> I think C probably requires us to allow this under an explicit cast, but we 
> can at least warn about it.  It does not require us to allow this as an 
> implicit conversion, and that should be an error.


Are you referring to casts to and from `void*`? I think the standard says those 
have to be allowed, and I don't see anything about whether or not the 
conversion has to be explicit.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I think C probably requires us to allow this under an explicit cast, but we can 
at least warn about it.  It does not require us to allow this as an implicit 
conversion, and that should be an error.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

If nobody else agrees with my position on this, I'm not going to continue 
arguing on the explicit cast behavior.  But please add a testcase showing that 
at least `(global int**)(void*)(local int**)p` works without an error.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D58236#1437690 , @ebevhan wrote:

> Any more input on this?
>
> I could redo the patch to simply fix the bug and not make the conversions 
> stricter, if that's preferable.


I was playing a bit with some examples of enum with pointer field of various 
address spaces and I couldn't find any case where successfully converting 
nested pointers was useful. Instead it could easily result in execution of 
incorrect code. So I am sticking to the opinion to be more strict.

Similarly, C++ is more strict with similar conversions i.e. nested pointers 
between Derived and Base. I still think since we are implementing new rules it 
is good to be more helpful rather than inheriting older logic of permitting 
everything like in C.

Also of course if we find later any issue we could fix them and modify the 
behavior.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Any more input on this?

I could redo the patch to simply fix the bug and not make the conversions 
stricter, if that's preferable.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D58236#1417429 , @efriedma wrote:

> In D58236#1416765 , @Anastasia wrote:
>
> > In D58236#1414069 , @efriedma 
> > wrote:
> >
> > > > I think trying to reject code that is doing something dangerous is a 
> > > > good thing!
> > >
> > > Refusing to compile code which is suspicious, but not forbidden by the 
> > > specification, will likely cause compatibility issues; there are 
> > > legitimate reasons to use casts which look weird.
> >
> >
> > The spec dioesn't allow these conversions either, it just simply doesn't 
> > cover this corner case at all. I don't think we are changing anything in 
> > terms of compatibility. If you have any examples of such casts that can be 
> > legitimate I would like to understand them better. What I have seen so far 
> > were the examples where `addrspacecast` was lost in IR for the memory 
> > segments translation and therefore wrong memory areas were accessed.
>
>
> The spec just says that the casts follow C rules... and C says you can cast a 
> pointer to an object type to a pointer to another object type (subject to 
> alignment restrictions).  By default, a pointer to a pointer isn't special.
>
> In practice, unusual casts tend to show up in code building a datastructure 
> using union-like constructs.  In plain C, for example, sometimes you have a 
> pointer to a float, and sometimes you have a pointer to an int, determined 
> dynamically.  I expect similar cases show up where a pointer points to memory 
> which contains either a pointer in the global address-space, or a pointer in 
> the local address-space, determined dynamically.  In some cases, it might be 
> clearer to use void* in more places, but that's mostly style issue.


Ok, do you have any example for pointers with different types by some chance? 
It would be very helpful because I can try to see what would be the behavior 
with different address spaces...

One fundamental difference between C and OpenCL C is that we wanted to give 
more clear semantic to address spaces. It is not possible to casts between 
pointers of arbitrary address spaces. We wanted to make such conversions very 
explicit using generic address space. If the code is written using unions (or 
some other way) that requires such "meaningless" conversions it can still be 
cast but the cast has to be written using pointer indirection with generic 
address spaces. That was done deliberately to prevent accidental erroneous 
patterns to be compiled.

> 
> 
>>> But that's a separate issue, and it needs a proper cost-benefit analysis, 
>>> including an analysis of the false-positive rate on existing code.
>> 
>> Do you have any suggestions how to do this in practice with such rare corner 
>> case?
> 
> If the warning never triggers on any code you have access to, that would 
> still be a useful datapoint.

To my knowledge these corner cases hasn't occurred yet in any code pattern 
known to me up. I created this upstream bug myself while sorting out some other 
address space related aspects. However, it's not impossible that it might 
happen in the future.

I still feel this might be the right direction for OpenCL, do you think this 
might not be right for C? If that's the case potentially we could add OpenCL 
specific checks for now and then try to clarify what should be the right 
strategy for C... it feels in C we just converts between addr spaces freely. 
The only danger of this conversion with nested pointers is that it produces 
`bitcast` instead of `addrspacecast`, which might result in accessing the wrong 
memory. Therefore, we wanted to restrict this in the first place.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D58236#1416765 , @Anastasia wrote:

> In D58236#1414069 , @efriedma wrote:
>
> > > I think trying to reject code that is doing something dangerous is a good 
> > > thing!
> >
> > Refusing to compile code which is suspicious, but not forbidden by the 
> > specification, will likely cause compatibility issues; there are legitimate 
> > reasons to use casts which look weird.
>
>
> The spec dioesn't allow these conversions either, it just simply doesn't 
> cover this corner case at all. I don't think we are changing anything in 
> terms of compatibility. If you have any examples of such casts that can be 
> legitimate I would like to understand them better. What I have seen so far 
> were the examples where `addrspacecast` was lost in IR for the memory 
> segments translation and therefore wrong memory areas were accessed.


The spec just says that the casts follow C rules... and C says you can cast a 
pointer to an object type to a pointer to another object type (subject to 
alignment restrictions).  By default, a pointer to a pointer isn't special.

In practice, unusual casts tend to show up in code building a datastructure 
using union-like constructs.  In plain C, for example, sometimes you have a 
pointer to a float, and sometimes you have a pointer to an int, determined 
dynamically.  I expect similar cases show up where a pointer points to memory 
which contains either a pointer in the global address-space, or a pointer in 
the local address-space, determined dynamically.  In some cases, it might be 
clearer to use void* in more places, but that's mostly style issue.

>> I'm not against adding some sort of address space suspicious cast warning to 
>> catch cases where we think the user meant to do something else.
> 
> I simply don't see how these conversions can be useful and some are 
> definitely indirectly forbidden (there is no precise wording however). There 
> are other ways to perform such conversions differently (by being more 
> explicit) where correct IR can be then generated with `addrspacecast`. I 
> don't think we are loosing anything in terms of functionality.

In some cases, the correct code may not involve an addrspacecast at all; the 
pointer could just have the "wrong" pointee type, and the code expects to 
explicitly fix it.  In that case, how do you fix it?  Write `(foo*)(void*)p` 
instead of `(foo*)p`?

>> But that's a separate issue, and it needs a proper cost-benefit analysis, 
>> including an analysis of the false-positive rate on existing code.
> 
> Do you have any suggestions how to do this in practice with such rare corner 
> case?

If the warning never triggers on any code you have access to, that would still 
be a useful datapoint.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D58236#1414069 , @efriedma wrote:

> > I think trying to reject code that is doing something dangerous is a good 
> > thing!
>
> Refusing to compile code which is suspicious, but not forbidden by the 
> specification, will likely cause compatibility issues; there are legitimate 
> reasons to use casts which look weird.


The spec dioesn't allow these conversions either, it just simply doesn't cover 
this corner case at all. I don't think we are changing anything in terms of 
compatibility. If you have any examples of such casts that can be legitimate I 
would like to understand them better. What I have seen so far were the examples 
where `addrspacecast` was lost in IR for the memory segments translation and 
therefore wrong memory areas were accessed.

> I'm not against adding some sort of address space suspicious cast warning to 
> catch cases where we think the user meant to do something else.

I simply don't see how these conversions can be useful and some are definitely 
indirectly forbidden (there is no precise wording however). There are other 
ways to perform such conversions differently (by being more explicit) where 
correct IR can be then generated with `addrspacecast`. I don't think we are 
loosing anything in terms of functionality.

> But that's a separate issue, and it needs a proper cost-benefit analysis, 
> including an analysis of the false-positive rate on existing code.

Do you have any suggestions how to do this in practice with such rare corner 
case?


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> I think trying to reject code that is doing something dangerous is a good 
> thing!

Refusing to compile code which is suspicious, but not forbidden by the 
specification, will likely cause compatibility issues; there are legitimate 
reasons to use casts which look weird.

I'm not against adding some sort of address space suspicious cast warning to 
catch cases where we think the user meant to do something else.  But that's a 
separate issue, and it needs a proper cost-benefit analysis, including an 
analysis of the false-positive rate on existing code.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

> Along those lines, in general, the normal C rules should allow casting `foo*` 
> to `bar*` for any object types foo and bar, even if foo and bar are pointers 
> with address spaces, like `__local int *` and `__global int *`.  I don't see 
> anything in the OpenCL standard that would contradict this.  It looks like 
> this patch changes that?

s6.5 of OpenCL spec is very explicit about this case: "A pointer  to  address  
space  A  can  only  be  assigned  to  a  pointer  to  the  same  address  
space  A  or  a pointer  to  the  generic  address  space.  Casting  a  pointer 
 to  address  space  A  to  a  pointer  to  address space B is illegal if A and 
B are named address spaces and A is not the same as B". Although there is a 
typo with missing first occurrence of B.

The behavior with generic is explained in s6.5.5.

Btw, this patch doesn't change anything in the address space conversions of 
pointers in OpenCL in general but just changes the behavior of nested pointers 
for which spec doesn't say anything. I think trying to reject code that is 
doing something dangerous is a good thing!


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D58236#1412267 , @Anastasia wrote:

> LGTM! Thanks a lot for fixing this old bug! Btw, do you plan to look at 
> generalizing this to C++ as well?


That does sound like a good idea and I will probably look into it when I have 
more time. I'd also like to look into making an RFC for the improved address 
space specification support. Again, when I have time :)

In D58236#1412324 , @efriedma wrote:

> Generally, with an explicit cast, C allows any pointer cast with a reasonable 
> interpretation, even if the underlying operation is suspicious.  For example, 
> you can cast an "long*" to a "int*" (as in "(int*)(long*)p") without any 
> complaint, even though dereferencing the result is likely undefined behavior. 
>  (C doesn't allow explicitly writing a reinterpret_cast, but that's basically 
> the operation in question.)


That is true... The current rules are very permissive about pointer casts.

> Along those lines, in general, the normal C rules should allow casting `foo*` 
> to `bar*` for any object types foo and bar, even if foo and bar are pointers 
> with address spaces, like `__local int *` and `__global int *`.  I don't see 
> anything in the OpenCL standard that would contradict this.  It looks like 
> this patch changes that?

I vaguely recall that when I was looking into fixing this in our downstream a 
while back I found that OpenCL did not have any explicit provisions for nested 
address spaces, only direct ones.

So should we drop the extra error on nested space mismatches, since the rules 
should be as permissive as possible unless the spec prohibits it? Technically 
the patch could be changed to only fix the incorrect warning on direct address 
space mismatches instead.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: test/SemaOpenCL/address-spaces.cl:87
+
+  // FIXME: This doesn't seem right. This should be an error, not a warning.
+  __local int * __global * __private * lll;

Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > Are we sure it has to be an error? May be we can change this wording to 
> > > something like - unknown behavior that needs clarifying? Some similar 
> > > text to your comment in the code.
> > Well... If `__private int **` to `__generic int **` is an error but 
> > `__private int ***` to `__generic int **` isn't, that seems a bit odd to 
> > me...
> > 
> Ok, I am still confused about this case because it's converting number of 
> pointers. It's not exactly the same to me as converting address spaces of 
> pointers. Since `__generic` is default addr space in OpenCL, in your example 
> following 2 inner address spaces seem to match:
> `__private int * __generic* __generic*` -> `__generic int * __generic*`
> 
> But in any case keeping FIXME is definitely the right thing to do here until 
> we come up with some sort of rules.
For reasons that have always been obscure to me, Clang is very permissive about 
pointer-type changes in C, far more than the C standard actually requires.  
It's hard to change that in general, but I think it's fine to harden specific 
new cases like nested address-space conversions.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Generally, with an explicit cast, C allows any pointer cast with a reasonable 
interpretation, even if the underlying operation is suspicious.  For example, 
you can cast an "long*" to a "int*" (as in "(int*)(long*)p") without any 
complaint, even though dereferencing the result is likely undefined behavior.  
(C doesn't allow explicitly writing a reinterpret_cast, but that's basically 
the operation in question.)

Along those lines, in general, the normal C rules should allow casting `foo*` 
to `bar*` for any object types foo and bar, even if foo and bar are pointers 
with address spaces, like `__local int *` and `__global int *`.  I don't see 
anything in the OpenCL standard that would contradict this.  It looks like this 
patch changes that?

I agree we shouldn't allow implicit conversions, though...


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks a lot for fixing this old bug! Btw, do you plan to look at 
generalizing this to C++ as well?

I don't feel we need anything for mixing OpenCL with GNU style address spaces 
at this point. We can always extend this in the future (in case it turns out to 
be useful to someone!).




Comment at: test/SemaOpenCL/address-spaces.cl:87
+
+  // FIXME: This doesn't seem right. This should be an error, not a warning.
+  __local int * __global * __private * lll;

ebevhan wrote:
> Anastasia wrote:
> > Are we sure it has to be an error? May be we can change this wording to 
> > something like - unknown behavior that needs clarifying? Some similar text 
> > to your comment in the code.
> Well... If `__private int **` to `__generic int **` is an error but 
> `__private int ***` to `__generic int **` isn't, that seems a bit odd to me...
> 
Ok, I am still confused about this case because it's converting number of 
pointers. It's not exactly the same to me as converting address spaces of 
pointers. Since `__generic` is default addr space in OpenCL, in your example 
following 2 inner address spaces seem to match:
`__private int * __generic* __generic*` -> `__generic int * __generic*`

But in any case keeping FIXME is definitely the right thing to do here until we 
come up with some sort of rules.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: test/SemaOpenCL/address-spaces.cl:87
+
+  // FIXME: This doesn't seem right. This should be an error, not a warning.
+  __local int * __global * __private * lll;

Anastasia wrote:
> Are we sure it has to be an error? May be we can change this wording to 
> something like - unknown behavior that needs clarifying? Some similar text to 
> your comment in the code.
Well... If `__private int **` to `__generic int **` is an error but `__private 
int ***` to `__generic int **` isn't, that seems a bit odd to me...



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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 188504.
ebevhan marked an inline comment as done.

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

https://reviews.llvm.org/D58236

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/numbered-address-space.cl
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/event_t_overload.cl
  test/SemaOpenCL/numbered-address-space.cl
  test/SemaOpenCL/queue_t_overload.cl

Index: test/SemaOpenCL/queue_t_overload.cl
===
--- test/SemaOpenCL/queue_t_overload.cl
+++ test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- test/SemaOpenCL/numbered-address-space.cl
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -7,5 +7,5 @@
   event_t evt;
   foo(evt, src1);
   foo(0, src2);
-  foo(evt, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(evt, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -52,6 +52,98 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of nested pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes address space of pointer}}
+
+  ll = g;// expected-error {{assigning '__global int *' to '__local int **' changes address space of pointer}}
+  ll = gg;   // expected-error {{assigning '__global int **' to '__local int **' changes address space of nested pointer}}
+  ll = l;// expected-error {{assigning '__local int *' to '__local int **' changes address space of pointer}}
+  ll = gg_f; // expected-error {{assigning '__global float **' to '__local int **' changes address space of nested pointer}}
+  ll = (__local int * __private *)gg_f; // expected-error {{casting '__global float **' to type '__local int **' changes address space of nested pointer}}
+
+  gg_f = 

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:7795
+if (lhq.getAddressSpace() != rhq.getAddressSpace())
+  return Sema::IncompatibleNestedPointerDiscardsQualifiers;
+

I am wondering since the behavior is so specialized for the address spaces 
whether we should name this something like 
`IncompatibleNestedPointerAddressSpaceMismatch`.



Comment at: test/SemaOpenCL/address-spaces.cl:87
+
+  // FIXME: This doesn't seem right. This should be an error, not a warning.
+  __local int * __global * __private * lll;

Are we sure it has to be an error? May be we can change this wording to 
something like - unknown behavior that needs clarifying? Some similar text to 
your comment in the code.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 2 inline comments as done.
ebevhan added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+  "|%diff{casting $ to type $|casting between types}0,1}2"
+  " changes address space of nested pointer">;
 def err_typecheck_incompatible_ownership : Error<

Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > I am wondering if we could just unify with the diagnostic above?
> > > 
> > > We could add another select at the end:
> > >   " changes address space of %select{nested|}3 pointer"
> > That is doable, but all of the 'typecheck' errors/warnings are made to be 
> > regular. If I add another parameter, there needs to be a special case in 
> > DiagnoseAssignmentResult for that error in particular.
> Oh I see... might not worth it?
I think keeping the generality makes it a bit simpler. Technically many of the 
errors here could be folded into one or two instead, but that hasn't been done, 
so...



Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to 
'__local int *__global **' from '__global int **'}}
+}

Anastasia wrote:
> ebevhan wrote:
> > ebevhan wrote:
> > > Anastasia wrote:
> > > > ebevhan wrote:
> > > > > This doesn't seem entirely correct still, but I'm not sure what to do 
> > > > > about it.
> > > > Is it because `Sema::IncompatiblePointer` has priority? We might want 
> > > > to change that. I think it was ok before because qualifier's mismatch 
> > > > was only a warning but now with the address spaces we are giving an 
> > > > error. I wonder if adding a separate enum item for address spaces 
> > > > (something like `Sema::IncompatibleNestedPointerAddressSpace`) would 
> > > > simplify things.
> > > > Is it because `Sema::IncompatiblePointer` has priority?
> > > 
> > > Sort of. The problem is that the AS pointee qualifiers match up until the 
> > > 'end' of the RHS pointer chain (LHS: `private->global->local`, RHS: 
> > > `private->global`), so we never get an 'incompatible address space' to 
> > > begin with. We only get that if 1) the bottommost type is equal after 
> > > unwrapping pointers (as far as both sides go), or 2) any of the 'shared' 
> > > AS qualifiers (as far as both sides go) were different.
> > > 
> > > The idea is that stopping when either side is no longer a pointer will 
> > > produce 'incompatible pointers' when you have different pointer depths, 
> > > but it doesn't consider anything below the 'shallowest' side of the 
> > > pointer chain, so we miss out on any AS mismatches further down.
> > > 
> > > (Not that there's anything to mismatch, really. There is no matching 
> > > pointer on the other side, so what is really the error?)
> > > 
> > > What should the criteria be for when the pointer types 'run out'? I could 
> > > have it keep digging through the other pointer until it hits a different 
> > > AS? This would mean that this:
> > > ```
> > > int  a;
> > > int ** b = a;
> > > ```
> > > could give a different warning than it does today, though (incompatible 
> > > nested qualifiers instead of incompatible pointers, which doesn't make 
> > > sense...) . We would have to skip the `lhptee == rhptee` check if we 
> > > 'kept going' despite one side not being a pointer type. So I don't know 
> > > if that's the right approach in general.
> > > 
> > > Or should we be searching 'backwards' instead, starting from the 
> > > innermost pointee? I don't know.
> > > 
> > > It really feels like the whole `checkPointerTypesForAssignment` routine 
> > > and everything surrounding it is a bit messy. It relies on an implicit 
> > > result from another function (`typesAreCompatible`) and then tries to 
> > > deduce why that function thought the types weren't compatible. Then 
> > > another function later on (`DiagnoseAssignmentResult`) tries to deduce 
> > > why THIS function thought something was wrong.
> > > 
> > > > I wonder if adding a separate enum item for address spaces (something 
> > > > like `Sema::IncompatibleNestedPointerAddressSpace`) would simplify 
> > > > things.
> > > 
> > > This would simplify the logic on the error emission side, since we don't 
> > > need to duplicate the logic for determining what went wrong, but doesn't 
> > > help with diagnosing the actual problem. Probably a good idea to add it 
> > > anyway, I just wanted to avoid adding a new enum member since that means 
> > > updating a lot of code elsewhere.
> > > We only get that if 1) the bottommost type is equal after unwrapping 
> > > pointers (as far as both sides go), or 2) any of the 'shared' AS 
> > > qualifiers (as far as both sides go) were different.
> > 
> > Sorry, should only be 2) here. Was focused on the whole 'incompatible 
> > nested qualifiers' result.
> > What should the criteria be for when the pointer types 'run out'? I could 
> > have it keep digging 

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 187531.

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

https://reviews.llvm.org/D58236

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/numbered-address-space.cl
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/event_t_overload.cl
  test/SemaOpenCL/numbered-address-space.cl
  test/SemaOpenCL/queue_t_overload.cl

Index: test/SemaOpenCL/queue_t_overload.cl
===
--- test/SemaOpenCL/queue_t_overload.cl
+++ test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- test/SemaOpenCL/numbered-address-space.cl
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -7,5 +7,5 @@
   event_t evt;
   foo(evt, src1);
   foo(0, src2);
-  foo(evt, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(evt, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -52,6 +52,98 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of nested pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes address space of pointer}}
+
+  ll = g;// expected-error {{assigning '__global int *' to '__local int **' changes address space of pointer}}
+  ll = gg;   // expected-error {{assigning '__global int **' to '__local int **' changes address space of nested pointer}}
+  ll = l;// expected-error {{assigning '__local int *' to '__local int **' changes address space of pointer}}
+  ll = gg_f; // expected-error {{assigning '__global float **' to '__local int **' changes address space of nested pointer}}
+  ll = (__local int * __private *)gg_f; // expected-error {{casting '__global float **' to type '__local int **' changes address space of nested pointer}}
+
+  gg_f = g;  // expected-error {{assigning 

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a subscriber: arsenm.
Anastasia added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+  "|%diff{casting $ to type $|casting between types}0,1}2"
+  " changes address space of nested pointer">;
 def err_typecheck_incompatible_ownership : Error<

ebevhan wrote:
> Anastasia wrote:
> > I am wondering if we could just unify with the diagnostic above?
> > 
> > We could add another select at the end:
> >   " changes address space of %select{nested|}3 pointer"
> That is doable, but all of the 'typecheck' errors/warnings are made to be 
> regular. If I add another parameter, there needs to be a special case in 
> DiagnoseAssignmentResult for that error in particular.
Oh I see... might not worth it?



Comment at: test/CodeGenOpenCL/numbered-address-space.cl:17
-void test_numbered_as_to_builtin(__attribute__((address_space(42))) int 
*arbitary_numbered_ptr, float src) {
-  volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, 
src, 0, 0, false);
-}

ebevhan wrote:
> Anastasia wrote:
> > Does this not compile any more?
> No, these tests were a bit shaky. I'm not even sure what they're supposed to 
> be testing. It's trying to pass an arbitrary `AS int *` pointer to a function 
> that takes `__local float *`. That AS conversion is illegal (implicitly), but 
> the illegal conversion was 'shadowed' by the 'incompatible pointer' warning, 
> so we didn't get an error. This is one of the things this patch fixes.
> 
> Since it's a codegen test, it should be producing something, but I'm not 
> really sure what is interesting to produce here, so I just removed it.
Ok, I will just loop in @arsenm  to confirm. OpenCL doesn't regulate arbitrary 
address spaces. And C doesn't regulate OpenCL ones. So interplay between those 
two has undefined behavior in my opinion. However, OpenCL code can make use of 
arbitrary address spaces since it's a valid Clang extension... But I am not 
sure what happens with this undefined behaviors.

For this specific case I would rather expect an error... but not sure it's 
worth testing this anyway.

May be Matt can provide us some more insights!



Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to 
'__local int *__global **' from '__global int **'}}
+}

ebevhan wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > ebevhan wrote:
> > > > This doesn't seem entirely correct still, but I'm not sure what to do 
> > > > about it.
> > > Is it because `Sema::IncompatiblePointer` has priority? We might want to 
> > > change that. I think it was ok before because qualifier's mismatch was 
> > > only a warning but now with the address spaces we are giving an error. I 
> > > wonder if adding a separate enum item for address spaces (something like 
> > > `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> > > Is it because `Sema::IncompatiblePointer` has priority?
> > 
> > Sort of. The problem is that the AS pointee qualifiers match up until the 
> > 'end' of the RHS pointer chain (LHS: `private->global->local`, RHS: 
> > `private->global`), so we never get an 'incompatible address space' to 
> > begin with. We only get that if 1) the bottommost type is equal after 
> > unwrapping pointers (as far as both sides go), or 2) any of the 'shared' AS 
> > qualifiers (as far as both sides go) were different.
> > 
> > The idea is that stopping when either side is no longer a pointer will 
> > produce 'incompatible pointers' when you have different pointer depths, but 
> > it doesn't consider anything below the 'shallowest' side of the pointer 
> > chain, so we miss out on any AS mismatches further down.
> > 
> > (Not that there's anything to mismatch, really. There is no matching 
> > pointer on the other side, so what is really the error?)
> > 
> > What should the criteria be for when the pointer types 'run out'? I could 
> > have it keep digging through the other pointer until it hits a different 
> > AS? This would mean that this:
> > ```
> > int  a;
> > int ** b = a;
> > ```
> > could give a different warning than it does today, though (incompatible 
> > nested qualifiers instead of incompatible pointers, which doesn't make 
> > sense...) . We would have to skip the `lhptee == rhptee` check if we 'kept 
> > going' despite one side not being a pointer type. So I don't know if that's 
> > the right approach in general.
> > 
> > Or should we be searching 'backwards' instead, starting from the innermost 
> > pointee? I don't know.
> > 
> > It really feels like the whole `checkPointerTypesForAssignment` routine and 
> > everything surrounding it is a bit messy. It relies on an implicit result 
> > from another function (`typesAreCompatible`) and then tries to deduce why 
> > that function thought the types weren't 

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to 
'__local int *__global **' from '__global int **'}}
+}

ebevhan wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > This doesn't seem entirely correct still, but I'm not sure what to do 
> > > about it.
> > Is it because `Sema::IncompatiblePointer` has priority? We might want to 
> > change that. I think it was ok before because qualifier's mismatch was only 
> > a warning but now with the address spaces we are giving an error. I wonder 
> > if adding a separate enum item for address spaces (something like 
> > `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> > Is it because `Sema::IncompatiblePointer` has priority?
> 
> Sort of. The problem is that the AS pointee qualifiers match up until the 
> 'end' of the RHS pointer chain (LHS: `private->global->local`, RHS: 
> `private->global`), so we never get an 'incompatible address space' to begin 
> with. We only get that if 1) the bottommost type is equal after unwrapping 
> pointers (as far as both sides go), or 2) any of the 'shared' AS qualifiers 
> (as far as both sides go) were different.
> 
> The idea is that stopping when either side is no longer a pointer will 
> produce 'incompatible pointers' when you have different pointer depths, but 
> it doesn't consider anything below the 'shallowest' side of the pointer 
> chain, so we miss out on any AS mismatches further down.
> 
> (Not that there's anything to mismatch, really. There is no matching pointer 
> on the other side, so what is really the error?)
> 
> What should the criteria be for when the pointer types 'run out'? I could 
> have it keep digging through the other pointer until it hits a different AS? 
> This would mean that this:
> ```
> int  a;
> int ** b = a;
> ```
> could give a different warning than it does today, though (incompatible 
> nested qualifiers instead of incompatible pointers, which doesn't make 
> sense...) . We would have to skip the `lhptee == rhptee` check if we 'kept 
> going' despite one side not being a pointer type. So I don't know if that's 
> the right approach in general.
> 
> Or should we be searching 'backwards' instead, starting from the innermost 
> pointee? I don't know.
> 
> It really feels like the whole `checkPointerTypesForAssignment` routine and 
> everything surrounding it is a bit messy. It relies on an implicit result 
> from another function (`typesAreCompatible`) and then tries to deduce why 
> that function thought the types weren't compatible. Then another function 
> later on (`DiagnoseAssignmentResult`) tries to deduce why THIS function 
> thought something was wrong.
> 
> > I wonder if adding a separate enum item for address spaces (something like 
> > `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> 
> This would simplify the logic on the error emission side, since we don't need 
> to duplicate the logic for determining what went wrong, but doesn't help with 
> diagnosing the actual problem. Probably a good idea to add it anyway, I just 
> wanted to avoid adding a new enum member since that means updating a lot of 
> code elsewhere.
> We only get that if 1) the bottommost type is equal after unwrapping pointers 
> (as far as both sides go), or 2) any of the 'shared' AS qualifiers (as far as 
> both sides go) were different.

Sorry, should only be 2) here. Was focused on the whole 'incompatible nested 
qualifiers' result.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 5 inline comments as done.
ebevhan added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+  "|%diff{casting $ to type $|casting between types}0,1}2"
+  " changes address space of nested pointer">;
 def err_typecheck_incompatible_ownership : Error<

Anastasia wrote:
> I am wondering if we could just unify with the diagnostic above?
> 
> We could add another select at the end:
>   " changes address space of %select{nested|}3 pointer"
That is doable, but all of the 'typecheck' errors/warnings are made to be 
regular. If I add another parameter, there needs to be a special case in 
DiagnoseAssignmentResult for that error in particular.



Comment at: lib/Sema/SemaCast.cpp:2294
+
+  // FIXME: C++ might want to emit different errors here.
   if (Self.getLangOpts().OpenCL) {

Anastasia wrote:
> I think my patch is divorcing C++ from here https://reviews.llvm.org/D58346.
> 
> Although, I might need to update it to add the same checks. I can do it once 
> your patch is finalized. :)
Yes, I noticed that you weren't calling checkAddressSpaceCast any longer in one 
of the code paths.

I suspect that your patch might already be doing the same thing... though not 
explicitly. TryAddressSpaceCast will only check compatibility on the top level 
pointee, and then it goes on to check type similarity, and if the types are not 
similar, it bails. Types with different nested pointer address spaces are not 
(should not be?) considered compatible, so we should get the same outcome. We 
won't get the 'nested' error, though.



Comment at: lib/Sema/SemaExpr.cpp:14199
+// qualifiers.
+// XXX: Canonical types?
+const Type *SrcPtr = SrcType->getPointeeType().getTypePtr();

Anastasia wrote:
> May be, since if we are using a typedef that is a pointer type 
> `isPointerType()` might not return true? I would just extend a test case with 
> typedef to a pointer type as one of the nested levels.
It sort of depends on what `checkPointerTypesForAssignment` considers invalid.

I think maybe I should throw this logic away and instead simply add a new enum 
member to AssignmentResult.



Comment at: test/CodeGenOpenCL/numbered-address-space.cl:17
-void test_numbered_as_to_builtin(__attribute__((address_space(42))) int 
*arbitary_numbered_ptr, float src) {
-  volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, 
src, 0, 0, false);
-}

Anastasia wrote:
> Does this not compile any more?
No, these tests were a bit shaky. I'm not even sure what they're supposed to be 
testing. It's trying to pass an arbitrary `AS int *` pointer to a function that 
takes `__local float *`. That AS conversion is illegal (implicitly), but the 
illegal conversion was 'shadowed' by the 'incompatible pointer' warning, so we 
didn't get an error. This is one of the things this patch fixes.

Since it's a codegen test, it should be producing something, but I'm not really 
sure what is interesting to produce here, so I just removed it.



Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to 
'__local int *__global **' from '__global int **'}}
+}

Anastasia wrote:
> ebevhan wrote:
> > This doesn't seem entirely correct still, but I'm not sure what to do about 
> > it.
> Is it because `Sema::IncompatiblePointer` has priority? We might want to 
> change that. I think it was ok before because qualifier's mismatch was only a 
> warning but now with the address spaces we are giving an error. I wonder if 
> adding a separate enum item for address spaces (something like 
> `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.
> Is it because `Sema::IncompatiblePointer` has priority?

Sort of. The problem is that the AS pointee qualifiers match up until the 'end' 
of the RHS pointer chain (LHS: `private->global->local`, RHS: 
`private->global`), so we never get an 'incompatible address space' to begin 
with. We only get that if 1) the bottommost type is equal after unwrapping 
pointers (as far as both sides go), or 2) any of the 'shared' AS qualifiers (as 
far as both sides go) were different.

The idea is that stopping when either side is no longer a pointer will produce 
'incompatible pointers' when you have different pointer depths, but it doesn't 
consider anything below the 'shallowest' side of the pointer chain, so we miss 
out on any AS mismatches further down.

(Not that there's anything to mismatch, really. There is no matching pointer on 
the other side, so what is really the error?)

What should the criteria be for when the pointer types 'run out'? I could have 
it keep digging through the other pointer until it hits a different AS? This 
would mean that this:
```
int  a;
int ** b = a;
```
could give a 

[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+  "|%diff{casting $ to type $|casting between types}0,1}2"
+  " changes address space of nested pointer">;
 def err_typecheck_incompatible_ownership : Error<

I am wondering if we could just unify with the diagnostic above?

We could add another select at the end:
  " changes address space of %select{nested|}3 pointer"



Comment at: lib/Sema/SemaCast.cpp:2294
+
+  // FIXME: C++ might want to emit different errors here.
   if (Self.getLangOpts().OpenCL) {

I think my patch is divorcing C++ from here https://reviews.llvm.org/D58346.

Although, I might need to update it to add the same checks. I can do it once 
your patch is finalized. :)



Comment at: lib/Sema/SemaCast.cpp:2295
+  // FIXME: C++ might want to emit different errors here.
   if (Self.getLangOpts().OpenCL) {
+const Type *DestPtr, *SrcPtr;

ebevhan wrote:
> I'd also like to petition to remove the guard on OpenCL here. Maybe an RFC 
> for formalizing the support for address space conversion semantics is in 
> order?
Yes, I'd support that! It would be good to generalize the rules as much as we 
can rather than adding extra checks for languages (the semantic is very 
similar).

As a matter of fact I tried to do the same in https://reviews.llvm.org/D58346 
in `TryAddressSpaceCast` and some C++ tests fail. So I had to add OpenCL check 
for now. :( Perhaps, they could just be changed but needs agreement with the 
community first.



Comment at: lib/Sema/SemaExpr.cpp:14199
+// qualifiers.
+// XXX: Canonical types?
+const Type *SrcPtr = SrcType->getPointeeType().getTypePtr();

May be, since if we are using a typedef that is a pointer type 
`isPointerType()` might not return true? I would just extend a test case with 
typedef to a pointer type as one of the nested levels.



Comment at: test/CodeGenOpenCL/numbered-address-space.cl:17
-void test_numbered_as_to_builtin(__attribute__((address_space(42))) int 
*arbitary_numbered_ptr, float src) {
-  volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, 
src, 0, 0, false);
-}

Does this not compile any more?



Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to 
'__local int *__global **' from '__global int **'}}
+}

ebevhan wrote:
> This doesn't seem entirely correct still, but I'm not sure what to do about 
> it.
Is it because `Sema::IncompatiblePointer` has priority? We might want to change 
that. I think it was ok before because qualifier's mismatch was only a warning 
but now with the address spaces we are giving an error. I wonder if adding a 
separate enum item for address spaces (something like 
`Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 6 inline comments as done.
ebevhan added inline comments.



Comment at: lib/Sema/SemaCast.cpp:2295
+  // FIXME: C++ might want to emit different errors here.
   if (Self.getLangOpts().OpenCL) {
+const Type *DestPtr, *SrcPtr;

I'd also like to petition to remove the guard on OpenCL here. Maybe an RFC for 
formalizing the support for address space conversion semantics is in order?



Comment at: lib/Sema/SemaExpr.cpp:14206
+// XXX: Should this be a different variation of the error, like
+// 'changes address space in nested pointer qualifiers'?
+DiagKind = diag::err_typecheck_incompatible_address_space;

rjmccall wrote:
> Yeah, I think that would be more straightforward.
I ended up not making it that different from the original one. Perhaps it's not 
different enough?


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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 187230.

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

https://reviews.llvm.org/D58236

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/numbered-address-space.cl
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/event_t_overload.cl
  test/SemaOpenCL/numbered-address-space.cl
  test/SemaOpenCL/queue_t_overload.cl

Index: test/SemaOpenCL/queue_t_overload.cl
===
--- test/SemaOpenCL/queue_t_overload.cl
+++ test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- test/SemaOpenCL/numbered-address-space.cl
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -7,5 +7,5 @@
   event_t evt;
   foo(evt, src1);
   foo(0, src2);
-  foo(evt, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(evt, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -52,6 +52,76 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of nested pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes address space of pointer}}
+
+  ll = g;// expected-error {{assigning '__global int *' to '__local int **' changes address space of pointer}}
+  ll = gg;   // expected-error {{assigning '__global int **' to '__local int **' changes address space of nested pointer}}
+  ll = l;// expected-error {{assigning '__local int *' to '__local int **' changes address space of pointer}}
+  ll = gg_f; // expected-error {{assigning '__global float **' to '__local int **' changes address space of nested pointer}}
+  ll = (__local int * __private *)gg_f; // expected-error {{casting '__global float **' to type '__local int **' changes address space of nested pointer}}
+
+  gg_f = g;  // expected-error {{assigning '__global int *' to '__global float 

[PATCH] D58236: Make address space conversions a bit stricter.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaCast.cpp:2306
+ SrcPPointee.getAddressSpace()) ||
+  !DestPPtr->isAddressSpaceOverlapping(*SrcPPtr)) {
+Self.Diag(OpRange.getBegin(),

This should `if (Nested ? DestPPointee.getAddressSpace() != 
SrcPPointee.getAddressSpace() : 
!DestPPtr->isAddressSpaceOverlapping(*SrcPtr))`, I think.



Comment at: lib/Sema/SemaExpr.cpp:7706
   if (!lhq.compatiblyIncludes(rhq)) {
 // Treat address-space mismatches as fatal.  TODO: address subspaces
 if (!lhq.isAddressSpaceSupersetOf(rhq))

The TODO here is fixed as much as anything else is.



Comment at: lib/Sema/SemaExpr.cpp:7781
   do {
+// Inconsistent address spaces at this point is invalid, even if the
+// address spaces would be compatible.

Please extract variables for `cast(lhptee)->getPointeeType()` (and 
for `rhptee`).



Comment at: lib/Sema/SemaExpr.cpp:14206
+// XXX: Should this be a different variation of the error, like
+// 'changes address space in nested pointer qualifiers'?
+DiagKind = diag::err_typecheck_incompatible_address_space;

Yeah, I think that would be more straightforward.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to 
'__local int *__global **' from '__global int **'}}
+}

This doesn't seem entirely correct still, but I'm not sure what to do about it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58236



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: Anastasia, rjmccall.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

The semantics for converting nested pointers between address
spaces are not very well defined. Some conversions which do not
really carry any meaning only produce warnings, and in some cases
warnings hide invalid conversions, such as 'global int*' to
'local float*'!

This patch changes the logic in checkPointerTypesForAssignment
and checkAddressSpaceCast to fail properly on conversions that
should definitely not be permitted. We also dig deeper into the
pointer types and fail on conversions where the address space
in a nested pointer changes, regardless of whether the address
space is compatible with the corresponding pointer nesting level
on the destination type.

See https://bugs.llvm.org/show_bug.cgi?id=39674


Repository:
  rC Clang

https://reviews.llvm.org/D58236

Files:
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/numbered-address-space.cl
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/event_t_overload.cl
  test/SemaOpenCL/numbered-address-space.cl
  test/SemaOpenCL/queue_t_overload.cl

Index: test/SemaOpenCL/queue_t_overload.cl
===
--- test/SemaOpenCL/queue_t_overload.cl
+++ test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- test/SemaOpenCL/numbered-address-space.cl
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -7,5 +7,5 @@
   event_t evt;
   foo(evt, src1);
   foo(0, src2);
-  foo(evt, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(evt, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -52,6 +52,76 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes