[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa29aa4710620: [OpenCL] Move addr space deduction to Sema. 
(authored by Anastasia).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D65744?vs=227841=231218#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65744

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaOpenCL/event_t.cl
  clang/test/SemaOpenCL/invalid-block.cl
  clang/test/SemaOpenCL/invalid-pipes-cl2.0.cl
  clang/test/SemaOpenCL/sampler_t.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/addrspace-auto.cl
  clang/test/SemaOpenCLCXX/restricted.cl

Index: clang/test/SemaOpenCLCXX/restricted.cl
===
--- clang/test/SemaOpenCLCXX/restricted.cl
+++ clang/test/SemaOpenCLCXX/restricted.cl
@@ -32,12 +32,14 @@
 __constant _Thread_local int a = 1;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '_Thread_local' storage class specifier}}
 // expected-warning@-2 {{'_Thread_local' is a C11 extension}}
-
+// expected-error@-3 {{thread-local storage is not supported for the current target}}
 __constant __thread int b = 2;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '__thread' storage class specifier}}
+// expected-error@-2 {{thread-local storage is not supported for the current target}}
 kernel void test_storage_classes() {
   register int x;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'register' storage class specifier}}
   thread_local int y;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'thread_local' storage class specifier}}
+  // expected-error@-2 {{thread-local storage is not supported for the current target}}
 }
Index: clang/test/SemaOpenCLCXX/addrspace-auto.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/addrspace-auto.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+__constant int i = 1;
+//CHECK: |-VarDecl {{.*}} ai '__global int':'__global int'
+auto ai = i;
+
+kernel void test() {
+  int i;
+  //CHECK: VarDecl {{.*}} ai 'int':'int'
+  auto ai = i;
+
+  constexpr int c = 1;
+  //CHECK: VarDecl {{.*}} used cai '__constant int':'__constant int'
+  __constant auto cai = c;
+  //CHECK: VarDecl {{.*}} aii 'int':'int'
+  auto aii = cai;
+
+  //CHECK: VarDecl {{.*}} ref 'int &'
+  auto  = i;
+  //CHECK: VarDecl {{.*}} ptr 'int *'
+  auto *ptr = 
+  //CHECK: VarDecl {{.*}} ref_c '__constant int &'
+  auto _c = cai;
+
+  //CHECK: VarDecl {{.*}} ptrptr 'int *__generic *'
+  auto **ptrptr = 
+  //CHECK: VarDecl {{.*}} refptr 'int *__generic &'
+  auto * = ptr;
+
+  //CHECK: VarDecl {{.*}} invalid gref '__global auto &'
+  __global auto  = i; //expected-error{{variable 'gref' with type '__global auto &' has incompatible initializer of type 'int'}}
+  __local int *ptr_l;
+  //CHECK: VarDecl {{.*}} invalid gptr '__global auto *'
+  __global auto *gptr = ptr_l; //expected-error{{variable 'gptr' with type '__global auto *' has incompatible initializer of type '__local int *'}}
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -65,30 +65,42 @@
 x3::x3(const x3 ) {}
 
 template 
-T xxx(T *in) {
+T xxx(T *in1, T in2) {
   // This pointer can't be deduced to generic because addr space
   // will be taken from the template argument.
   //CHECK: `-VarDecl {{.*}} i 'T *' cinit
-  T *i = in;
+  T *i = in1;
   T ii;
+  __private T *ptr = 
+  ptr = 
   return *i;
 }
 
 __kernel void test() {
   int foo[10];
-  xxx([0]);
+  xxx<__private int>([0], foo[0]);
+  // FIXME: Template param deduction fails here because
+  // temporaries are not in the __private address space.
+  // It is probably reasonable to put them in __private
+  // considering that stack and function params are
+  // implicitly in __private.
+  // However, if temporaries are left in default addr
+  // space we should at least pretty print the __private
+  // addr space. Otherwise diagnostic apprears to be
+  // confusing.
+  //xxx([0], foo[0]);
 }
 
 // Addr space for pointer/reference to an array
-//CHECK: FunctionDecl {{.*}} t1 'void (const __generic float (&)[2])'
+//CHECK: FunctionDecl {{.*}} t1 'void (const float (__generic &)[2])'
 void t1(const float ()[2]);
-//CHECK: FunctionDecl {{.*}} t2 'void (const __generic float (*)[2])'

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Since you're moving this code anyway, can this be split into its own 
> > > > function?  I'm not sure if it's actually important that some of these 
> > > > failures return immediately and some of them fall through to later 
> > > > checks.
> > > >  I'm not sure if it's actually important that some of these failures 
> > > > return immediately and some of them fall through to later checks.
> > > 
> > > Yes, it looks a bit random. Do we have any guideline whether we should 
> > > return or keep diagnosing as long as possible?
> > > 
> > > 
> > If the user is likely to have made multiple independent errors, it's good 
> > to diagnose as many of them as possible.  But if it's just as likely that 
> > the user messed up in a single way that should've meant we didn't take this 
> > code path, then it's better to bail out early.
> > 
> > In this case, most of these diagnostics are looking for different special 
> > OpenCL types, and those are probably all independent  of each other.
> > 
> > ...it does occur to me to wonder if more of these checks should be looking 
> > through array types the way that the check for `half` does.  Presumably you 
> > shouldn't be able to declare global arrays of images or pipes if you can't 
> > declare non-array variables of them.
> > ...it does occur to me to wonder if more of these checks should be looking 
> > through array types the way that the check for half does. Presumably you 
> > shouldn't be able to declare global arrays of images or pipes if you can't 
> > declare non-array variables of them.
> 
> We actually provide dedicated diagnostic for all other types in 
> `Sema::BuildArrayType`. No idea why half is handled here I will try to 
> refactor that in a separate patch. 
Well, `half` is a perfectly reasonable type to have an array of.  I don't know 
why that's not equally true of images or pipes; are they assumed to have 
implicit trailing storage?  Anyway, if OpenCL says arrays of them are 
forbidden, we don't need to look through arrays; that's probably worth a 
comment, though.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Since you're moving this code anyway, can this be split into its own 
> > > function?  I'm not sure if it's actually important that some of these 
> > > failures return immediately and some of them fall through to later checks.
> > >  I'm not sure if it's actually important that some of these failures 
> > > return immediately and some of them fall through to later checks.
> > 
> > Yes, it looks a bit random. Do we have any guideline whether we should 
> > return or keep diagnosing as long as possible?
> > 
> > 
> If the user is likely to have made multiple independent errors, it's good to 
> diagnose as many of them as possible.  But if it's just as likely that the 
> user messed up in a single way that should've meant we didn't take this code 
> path, then it's better to bail out early.
> 
> In this case, most of these diagnostics are looking for different special 
> OpenCL types, and those are probably all independent  of each other.
> 
> ...it does occur to me to wonder if more of these checks should be looking 
> through array types the way that the check for `half` does.  Presumably you 
> shouldn't be able to declare global arrays of images or pipes if you can't 
> declare non-array variables of them.
> ...it does occur to me to wonder if more of these checks should be looking 
> through array types the way that the check for half does. Presumably you 
> shouldn't be able to declare global arrays of images or pipes if you can't 
> declare non-array variables of them.

We actually provide dedicated diagnostic for all other types in 
`Sema::BuildArrayType`. No idea why half is handled here I will try to refactor 
that in a separate patch. 


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 227841.
Anastasia marked an inline comment as done.
Anastasia added a comment.

- Factored OpenCL diagnostics out in a separate helper function
- Removed special case for ParenType


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

https://reviews.llvm.org/D65744

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaOpenCL/event_t.cl
  clang/test/SemaOpenCL/invalid-block.cl
  clang/test/SemaOpenCL/invalid-pipes-cl2.0.cl
  clang/test/SemaOpenCL/sampler_t.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/addrspace-auto.cl
  clang/test/SemaOpenCLCXX/restricted.cl

Index: clang/test/SemaOpenCLCXX/restricted.cl
===
--- clang/test/SemaOpenCLCXX/restricted.cl
+++ clang/test/SemaOpenCLCXX/restricted.cl
@@ -32,12 +32,14 @@
 __constant _Thread_local int a = 1;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '_Thread_local' storage class specifier}}
 // expected-warning@-2 {{'_Thread_local' is a C11 extension}}
-
+// expected-error@-3 {{thread-local storage is not supported for the current target}}
 __constant __thread int b = 2;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '__thread' storage class specifier}}
+// expected-error@-2 {{thread-local storage is not supported for the current target}}
 kernel void test_storage_classes() {
   register int x;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'register' storage class specifier}}
   thread_local int y;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'thread_local' storage class specifier}}
+  // expected-error@-2 {{thread-local storage is not supported for the current target}}
 }
Index: clang/test/SemaOpenCLCXX/addrspace-auto.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/addrspace-auto.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+__constant int i = 1;
+//CHECK: |-VarDecl {{.*}} ai '__global int':'__global int'
+auto ai = i;
+
+kernel void test() {
+  int i;
+  //CHECK: VarDecl {{.*}} ai 'int':'int'
+  auto ai = i;
+
+  constexpr int c = 1;
+  //CHECK: VarDecl {{.*}} used cai '__constant int':'__constant int'
+  __constant auto cai = c;
+  //CHECK: VarDecl {{.*}} aii 'int':'int'
+  auto aii = cai;
+
+  //CHECK: VarDecl {{.*}} ref 'int &'
+  auto  = i;
+  //CHECK: VarDecl {{.*}} ptr 'int *'
+  auto *ptr = 
+  //CHECK: VarDecl {{.*}} ref_c '__constant int &'
+  auto _c = cai;
+
+  //CHECK: VarDecl {{.*}} ptrptr 'int *__generic *'
+  auto **ptrptr = 
+  //CHECK: VarDecl {{.*}} refptr 'int *__generic &'
+  auto * = ptr;
+
+  //CHECK: VarDecl {{.*}} invalid gref '__global auto &'
+  __global auto  = i; //expected-error{{variable 'gref' with type '__global auto &' has incompatible initializer of type 'int'}}
+  __local int *ptr_l;
+  //CHECK: VarDecl {{.*}} invalid gptr '__global auto *'
+  __global auto *gptr = ptr_l; //expected-error{{variable 'gptr' with type '__global auto *' has incompatible initializer of type '__local int *'}}
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -65,30 +65,42 @@
 x3::x3(const x3 ) {}
 
 template 
-T xxx(T *in) {
+T xxx(T *in1, T in2) {
   // This pointer can't be deduced to generic because addr space
   // will be taken from the template argument.
   //CHECK: `-VarDecl {{.*}} i 'T *' cinit
-  T *i = in;
+  T *i = in1;
   T ii;
+  __private T *ptr = 
+  ptr = 
   return *i;
 }
 
 __kernel void test() {
   int foo[10];
-  xxx([0]);
+  xxx<__private int>([0], foo[0]);
+  // FIXME: Template param deduction fails here because
+  // temporaries are not in the __private address space.
+  // It is probably reasonable to put them in __private
+  // considering that stack and function params are
+  // implicitly in __private.
+  // However, if temporaries are left in default addr
+  // space we should at least pretty print the __private
+  // addr space. Otherwise diagnostic apprears to be
+  // confusing.
+  //xxx([0], foo[0]);
 }
 
 // Addr space for pointer/reference to an array
-//CHECK: FunctionDecl {{.*}} t1 'void (const __generic float (&)[2])'
+//CHECK: FunctionDecl {{.*}} t1 'void (const float (__generic &)[2])'
 void t1(const float ()[2]);
-//CHECK: FunctionDecl {{.*}} t2 'void (const __generic float (*)[2])'
+//CHECK: FunctionDecl {{.*}} t2 'void (const float (__generic *)[2])'
 void t2(const float (*fYZ)[2]);

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+

Anastasia wrote:
> rjmccall wrote:
> > Since you're moving this code anyway, can this be split into its own 
> > function?  I'm not sure if it's actually important that some of these 
> > failures return immediately and some of them fall through to later checks.
> >  I'm not sure if it's actually important that some of these failures return 
> > immediately and some of them fall through to later checks.
> 
> Yes, it looks a bit random. Do we have any guideline whether we should return 
> or keep diagnosing as long as possible?
> 
> 
If the user is likely to have made multiple independent errors, it's good to 
diagnose as many of them as possible.  But if it's just as likely that the user 
messed up in a single way that should've meant we didn't take this code path, 
then it's better to bail out early.

In this case, most of these diagnostics are looking for different special 
OpenCL types, and those are probably all independent  of each other.

...it does occur to me to wonder if more of these checks should be looking 
through array types the way that the check for `half` does.  Presumably you 
shouldn't be able to declare global arrays of images or pipes if you can't 
declare non-array variables of them.



Comment at: clang/lib/Sema/SemaType.cpp:7460
+  // the initializing expression type during the type deduction.
+  (T->isUndeducedAutoType() && IsPointee) ||
   // OpenCL spec v2.0 s6.9.b:

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Okay, I understand why you're doing this now, and it makes sense.  I 
> > > > would like to propose changing the entire way 
> > > > `deduceOpenCLImplicitAddrSpace` works.  Why don't we do it more like 
> > > > how ObjC ARC infers its implicit ownership qualifiers:
> > > > 
> > > > - In SemaType, we add the default address space to non-qualified, 
> > > > non-dependent, non-undeduced-`auto` pointees when parsing a pointer or 
> > > > reference type.
> > > > - In SemaType, we add the default address space to non-qualified 
> > > > pointees when building a pointer or reference type.
> > > > - We add the default address space at the top level when when building 
> > > > a variable.
> > > > 
> > > > Then all of this context-specific logic where we're looking at 
> > > > different declarator chunks and trying to infer the relationship of the 
> > > > current chunk to the overall type being parsed can just go away or get 
> > > > pushed into a more appropriate position.
> > > Ok, it mainly works, however I still need a bit of parsing horribleness 
> > > when deducing addr space of declarations with parenthesis  in 
> > > `GetFullTypeForDeclarator`. This is the case for block pointers or 
> > > pointers/references to arrays. It is incorrect to add address spaces on 
> > > ParenType while building a pointer or references so I have to detect this 
> > > as special case.
> > You can't add an address space outside a `ParenType`?  That seems odd; what 
> > problems are you seeing exactly?
> > 
> > If it's really just specific to `ParenType`, you could simply drill through 
> > them and then rebuild the `ParenType`s.
> > You can't add an address space outside a `ParenType`? That seems odd; what 
> > problems are you seeing exactly?
> 
> When we add addr space for a pointee and it's a `ParenType` the addr space 
> should actually be added to a first non-`ParenType` but not `ParenType` 
> itself. For example is we declare a reference to an array we want the array 
> type to have an address space not `ParenType`.
> 
> > If it's really just specific to ParenType, you could simply drill through 
> > them and then rebuild the ParenTypes.
> 
> Ok, in the case I explained above we would have to add address space to the 
> first non-`ParenTypes` and then rebuild all `ParenType`s. I will try that.
> 
ParenType is just a sugar node, and qualifiers on it should be treated 
identically to qualifiers on the inner type, just like a qualifier on a typedef 
name (i.e. `const int32_t`) would.  So unless you're seeing a real problem I 
wouldn't worry about it.

I could totally believe that it prints poorly, though.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+

rjmccall wrote:
> Since you're moving this code anyway, can this be split into its own 
> function?  I'm not sure if it's actually important that some of these 
> failures return immediately and some of them fall through to later checks.
>  I'm not sure if it's actually important that some of these failures return 
> immediately and some of them fall through to later checks.

Yes, it looks a bit random. Do we have any guideline whether we should return 
or keep diagnosing as long as possible?





Comment at: clang/lib/Sema/SemaType.cpp:7460
+  // the initializing expression type during the type deduction.
+  (T->isUndeducedAutoType() && IsPointee) ||
   // OpenCL spec v2.0 s6.9.b:

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Okay, I understand why you're doing this now, and it makes sense.  I 
> > > would like to propose changing the entire way 
> > > `deduceOpenCLImplicitAddrSpace` works.  Why don't we do it more like how 
> > > ObjC ARC infers its implicit ownership qualifiers:
> > > 
> > > - In SemaType, we add the default address space to non-qualified, 
> > > non-dependent, non-undeduced-`auto` pointees when parsing a pointer or 
> > > reference type.
> > > - In SemaType, we add the default address space to non-qualified pointees 
> > > when building a pointer or reference type.
> > > - We add the default address space at the top level when when building a 
> > > variable.
> > > 
> > > Then all of this context-specific logic where we're looking at different 
> > > declarator chunks and trying to infer the relationship of the current 
> > > chunk to the overall type being parsed can just go away or get pushed 
> > > into a more appropriate position.
> > Ok, it mainly works, however I still need a bit of parsing horribleness 
> > when deducing addr space of declarations with parenthesis  in 
> > `GetFullTypeForDeclarator`. This is the case for block pointers or 
> > pointers/references to arrays. It is incorrect to add address spaces on 
> > ParenType while building a pointer or references so I have to detect this 
> > as special case.
> You can't add an address space outside a `ParenType`?  That seems odd; what 
> problems are you seeing exactly?
> 
> If it's really just specific to `ParenType`, you could simply drill through 
> them and then rebuild the `ParenType`s.
> You can't add an address space outside a `ParenType`? That seems odd; what 
> problems are you seeing exactly?

When we add addr space for a pointee and it's a `ParenType` the addr space 
should actually be added to a first non-`ParenType` but not `ParenType` itself. 
For example is we declare a reference to an array we want the array type to 
have an address space not `ParenType`.

> If it's really just specific to ParenType, you could simply drill through 
> them and then rebuild the ParenTypes.

Ok, in the case I explained above we would have to add address space to the 
first non-`ParenTypes` and then rebuild all `ParenType`s. I will try that.



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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:5998
+QualType Type = Var->getType();
+if (Type->isSamplerT() || Type->isVoidType())
+  return;

Anastasia wrote:
> I don't seem to need a check for dependent or auto types because the 
> substitution happens using type info rather than getting type from the 
> declaration. Not sure if I should explain it here or add the checks just in 
> case?
If you have adequate test-case coverage (both inside and out of templates) then 
I don't think you need further explanation.



Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+

Since you're moving this code anyway, can this be split into its own function?  
I'm not sure if it's actually important that some of these failures return 
immediately and some of them fall through to later checks.



Comment at: clang/lib/Sema/SemaType.cpp:7460
+  // the initializing expression type during the type deduction.
+  (T->isUndeducedAutoType() && IsPointee) ||
   // OpenCL spec v2.0 s6.9.b:

Anastasia wrote:
> rjmccall wrote:
> > Okay, I understand why you're doing this now, and it makes sense.  I would 
> > like to propose changing the entire way `deduceOpenCLImplicitAddrSpace` 
> > works.  Why don't we do it more like how ObjC ARC infers its implicit 
> > ownership qualifiers:
> > 
> > - In SemaType, we add the default address space to non-qualified, 
> > non-dependent, non-undeduced-`auto` pointees when parsing a pointer or 
> > reference type.
> > - In SemaType, we add the default address space to non-qualified pointees 
> > when building a pointer or reference type.
> > - We add the default address space at the top level when when building a 
> > variable.
> > 
> > Then all of this context-specific logic where we're looking at different 
> > declarator chunks and trying to infer the relationship of the current chunk 
> > to the overall type being parsed can just go away or get pushed into a more 
> > appropriate position.
> Ok, it mainly works, however I still need a bit of parsing horribleness when 
> deducing addr space of declarations with parenthesis  in 
> `GetFullTypeForDeclarator`. This is the case for block pointers or 
> pointers/references to arrays. It is incorrect to add address spaces on 
> ParenType while building a pointer or references so I have to detect this as 
> special case.
You can't add an address space outside a `ParenType`?  That seems odd; what 
problems are you seeing exactly?

If it's really just specific to `ParenType`, you could simply drill through 
them and then rebuild the `ParenType`s.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaDecl.cpp:5998
+QualType Type = Var->getType();
+if (Type->isSamplerT() || Type->isVoidType())
+  return;

I don't seem to need a check for dependent or auto types because the 
substitution happens using type info rather than getting type from the 
declaration. Not sure if I should explain it here or add the checks just in 
case?



Comment at: clang/lib/Sema/SemaType.cpp:7460
+  // the initializing expression type during the type deduction.
+  (T->isUndeducedAutoType() && IsPointee) ||
   // OpenCL spec v2.0 s6.9.b:

rjmccall wrote:
> Okay, I understand why you're doing this now, and it makes sense.  I would 
> like to propose changing the entire way `deduceOpenCLImplicitAddrSpace` 
> works.  Why don't we do it more like how ObjC ARC infers its implicit 
> ownership qualifiers:
> 
> - In SemaType, we add the default address space to non-qualified, 
> non-dependent, non-undeduced-`auto` pointees when parsing a pointer or 
> reference type.
> - In SemaType, we add the default address space to non-qualified pointees 
> when building a pointer or reference type.
> - We add the default address space at the top level when when building a 
> variable.
> 
> Then all of this context-specific logic where we're looking at different 
> declarator chunks and trying to infer the relationship of the current chunk 
> to the overall type being parsed can just go away or get pushed into a more 
> appropriate position.
Ok, it mainly works, however I still need a bit of parsing horribleness when 
deducing addr space of declarations with parenthesis  in 
`GetFullTypeForDeclarator`. This is the case for block pointers or 
pointers/references to arrays. It is incorrect to add address spaces on 
ParenType while building a pointer or references so I have to detect this as 
special case.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 219903.
Anastasia added a comment.

- Move addr space deduction to a later phase.


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

https://reviews.llvm.org/D65744

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaOpenCL/event_t.cl
  clang/test/SemaOpenCL/invalid-block.cl
  clang/test/SemaOpenCL/invalid-pipes-cl2.0.cl
  clang/test/SemaOpenCL/sampler_t.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/addrspace-auto.cl
  clang/test/SemaOpenCLCXX/restricted.cl

Index: clang/test/SemaOpenCLCXX/restricted.cl
===
--- clang/test/SemaOpenCLCXX/restricted.cl
+++ clang/test/SemaOpenCLCXX/restricted.cl
@@ -32,12 +32,14 @@
 __constant _Thread_local int a = 1;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '_Thread_local' storage class specifier}}
 // expected-warning@-2 {{'_Thread_local' is a C11 extension}}
-
+// expected-error@-3 {{thread-local storage is not supported for the current target}}
 __constant __thread int b = 2;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '__thread' storage class specifier}}
+// expected-error@-2 {{thread-local storage is not supported for the current target}}
 kernel void test_storage_classes() {
   register int x;
-  // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'register' storage class specifier}}
+// expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'register' storage class specifier}}
   thread_local int y;
-  // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'thread_local' storage class specifier}}
+// expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'thread_local' storage class specifier}}
+// expected-error@-2 {{thread-local storage is not supported for the current target}}
 }
Index: clang/test/SemaOpenCLCXX/addrspace-auto.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/addrspace-auto.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+__constant int i = 1;
+//CHECK: |-VarDecl {{.*}} ai '__global int':'__global int'
+auto ai = i;
+
+kernel void test() {
+  int i;
+  //CHECK: VarDecl {{.*}} ai 'int':'int'
+  auto ai = i;
+
+  constexpr int c = 1;
+  //CHECK: VarDecl {{.*}} used cai '__constant int':'__constant int'
+  __constant auto cai = c;
+  //CHECK: VarDecl {{.*}} aii 'int':'int'
+  auto aii = cai;
+
+  //CHECK: VarDecl {{.*}} ref 'int &'
+  auto  = i;
+  //CHECK: VarDecl {{.*}} ptr 'int *'
+  auto *ptr = 
+  //CHECK: VarDecl {{.*}} ref_c '__constant int &'
+  auto _c = cai;
+
+  //CHECK: VarDecl {{.*}} ptrptr 'int *__generic *'
+  auto **ptrptr = 
+  //CHECK: VarDecl {{.*}} refptr 'int *__generic &'
+  auto * = ptr;
+
+  //CHECK: VarDecl {{.*}} invalid gref '__global auto &'
+  __global auto  = i; //expected-error{{variable 'gref' with type '__global auto &' has incompatible initializer of type 'int'}}
+  __local int *ptr_l;
+  //CHECK: VarDecl {{.*}} invalid gptr '__global auto *'
+  __global auto *gptr = ptr_l; //expected-error{{variable 'gptr' with type '__global auto *' has incompatible initializer of type '__local int *'}}
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -65,18 +65,30 @@
 x3::x3(const x3 ) {}
 
 template 
-T xxx(T *in) {
+T xxx(T *in1, T in2) {
   // This pointer can't be deduced to generic because addr space
   // will be taken from the template argument.
   //CHECK: `-VarDecl {{.*}} i 'T *' cinit
-  T *i = in;
+  T *i = in1;
   T ii;
+  __private T *ptr = 
+  ptr = 
   return *i;
 }
 
 __kernel void test() {
   int foo[10];
-  xxx([0]);
+  xxx<__private int>([0], foo[0]);
+  // FIXME: Template param deduction fails here because
+  // temporaries are not in the __private address space.
+  // It is probably reasonable to put them in __private
+  // considering that stack and function params are
+  // implicitly in __private.
+  // However, if temporaries are left in default addr
+  // space we should at least pretty print the __private
+  // addr space. Otherwise diagnostic apprears to be
+  // confusing.
+  //xxx([0], foo[0]);
 }
 
 // Addr space for pointer/reference to an array
Index: clang/test/SemaOpenCL/sampler_t.cl
===
--- clang/test/SemaOpenCL/sampler_t.cl
+++ clang/test/SemaOpenCL/sampler_t.cl
@@ -48,6 +48,9 @@
 sampler_t bad(void); 

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7460
+  // the initializing expression type during the type deduction.
+  (T->isUndeducedAutoType() && IsPointee) ||
   // OpenCL spec v2.0 s6.9.b:

Okay, I understand why you're doing this now, and it makes sense.  I would like 
to propose changing the entire way `deduceOpenCLImplicitAddrSpace` works.  Why 
don't we do it more like how ObjC ARC infers its implicit ownership qualifiers:

- In SemaType, we add the default address space to non-qualified, 
non-dependent, non-undeduced-`auto` pointees when parsing a pointer or 
reference type.
- In SemaType, we add the default address space to non-qualified pointees when 
building a pointer or reference type.
- We add the default address space at the top level when when building a 
variable.

Then all of this context-specific logic where we're looking at different 
declarator chunks and trying to infer the relationship of the current chunk to 
the overall type being parsed can just go away or get pushed into a more 
appropriate position.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 218661.
Anastasia added a comment.

- Added forgotten test


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

https://reviews.llvm.org/D65744

Files:
  clang/include/clang/AST/Type.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaOpenCLCXX/addrspace-auto.cl

Index: clang/test/SemaOpenCLCXX/addrspace-auto.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/addrspace-auto.cl
@@ -0,0 +1,31 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+kernel void test(){
+  int i;
+//CHECK: VarDecl {{.*}} ai 'int':'int'
+  auto ai = i;
+
+  constexpr int c = 1;
+//CHECK: VarDecl {{.*}} used cai '__constant int':'__constant int'
+  __constant auto cai = c;
+//CHECK: VarDecl {{.*}} aii 'int':'int'
+  auto aii = cai;
+
+//CHECK: VarDecl {{.*}} ref 'int &'
+  auto& ref = i;
+//CHECK: VarDecl {{.*}} ptr 'int *'
+  auto* ptr = 
+//CHECK: VarDecl {{.*}} ref_c '__constant int &'
+  auto& ref_c = cai;
+
+//CHECK: VarDecl {{.*}} ptrptr 'int *__generic *'
+  auto ** ptrptr = 
+//CHECK: VarDecl {{.*}} refptr 'int *__generic &'
+  auto *& refptr = ptr;
+
+//CHECK: VarDecl {{.*}} invalid gref '__global auto &'
+  __global auto& gref = i; //expected-error{{variable 'gref' with type '__global auto &' has incompatible initializer of type 'int'}}
+  __local int* ptr_l;
+//CHECK: VarDecl {{.*}} invalid gptr '__global auto *'
+  __global auto* gptr = ptr_l; //expected-error{{variable 'gptr' with type '__global auto *' has incompatible initializer of type '__local int *'}}
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -4535,14 +4535,6 @@
   return Result;
 }
 
-/// Helper to deduce addr space of a pointee type in OpenCL mode.
-/// If the type is updated it will be overwritten in PointeeType param.
-static void deduceOpenCLPointeeAddrSpace(Sema , QualType ) {
-  if (PointeeType.getAddressSpace() == LangAS::Default)
-PointeeType = SemaRef.Context.getAddrSpaceQualType(PointeeType,
-   LangAS::opencl_generic);
-}
-
 template
 QualType TreeTransform::TransformPointerType(TypeLocBuilder ,
   PointerTypeLoc TL) {
@@ -4551,9 +4543,6 @@
   if (PointeeType.isNull())
 return QualType();
 
-  if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
-
   QualType Result = TL.getType();
   if (PointeeType->getAs()) {
 // A dependent pointer type 'T *' has is being transformed such
@@ -4592,9 +4581,6 @@
   if (PointeeType.isNull())
 return QualType();
 
-  if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
-
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
   PointeeType != TL.getPointeeLoc().getType()) {
@@ -4624,9 +4610,6 @@
   if (PointeeType.isNull())
 return QualType();
 
-  if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
-
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
   PointeeType != T->getPointeeTypeAsWritten()) {
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1962,6 +1962,18 @@
   return true;
 }
 
+// Helper to deduce addr space of a pointee type in OpenCL mode.
+static QualType deduceOpenCLPointeeAddrSpace(Sema , QualType PointeeType) {
+  if (!PointeeType->isUndeducedAutoType() && !PointeeType->isDependentType() &&
+  !PointeeType.getQualifiers().hasAddressSpace())
+PointeeType = S.getASTContext().getAddrSpaceQualType(
+PointeeType,
+S.getLangOpts().OpenCLCPlusPlus || S.getLangOpts().OpenCLVersion == 200
+? LangAS::opencl_generic
+: LangAS::opencl_private);
+  return PointeeType;
+}
+
 /// Build a pointer type.
 ///
 /// \param T The type to which we'll be building a pointer.
@@ -1998,6 +2010,9 @@
   if (getLangOpts().ObjCAutoRefCount)
 T = inferARCLifetimeForPointee(*this, T, Loc, /*reference*/ false);
 
+  if (getLangOpts().OpenCL)
+T = deduceOpenCLPointeeAddrSpace(*this, T);
+
   // Build the pointer type.
   return Context.getPointerType(T);
 }
@@ -2058,6 +2073,9 @@
   if (getLangOpts().ObjCAutoRefCount)
 T = inferARCLifetimeForPointee(*this, T, Loc, /*reference*/ true);
 
+  if (getLangOpts().OpenCL)
+T = deduceOpenCLPointeeAddrSpace(*this, T);
+
   // Handle restrict on references.
   if (LValueRef)
 return Context.getLValueReferenceType(T, SpelledAsLValue);
@@ -2626,6 +2644,9 @@
   if (checkQualifiedFunction(*this, T, Loc, QFK_BlockPointer))
 return QualType();
 
+  if (getLangOpts().OpenCL)
+T = 

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D65744#1639175 , @mantognini wrote:

> I think this looks good. Maybe the tests should be extended to test `auto` as 
> function return type, and if there's some special handling around 
> `decltype(auto)`, then it should be tested too, but I'm not sure it's 
> actually needed here. What do you think?


Do you mean anything specific to test?


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/Type.h:6509
+  return isa(CanonicalType);
+}
+

rjmccall wrote:
> Hmm.  So this method, confusingly, will not return true for a deduced `auto`, 
> unless the deduced type is itself an undeduced `auto` (which I'm not sure can 
> happen).  I think it at least needs a different name; `isUndeducedAutoType()` 
> would be okay if the latter case is not possible.  But it might be better if 
> we can just define away the need for the method entirely.
I feel I still need this helper in order to detect the auto type. Alternatively 
I can create a static function for this instead of publicly exposing this 
functionality. Or maybe you have other ideas in mind...


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D65744#1653081 , @rjmccall wrote:

> In D65744#1652355 , @Anastasia wrote:
>
> > I don't think this is likely to change. Are you suggesting to move the 
> > deduction logic for pointee of pointers, references and block pointers into 
> > ASTContext helper that creates a pointer/reference/block pointer type?
>
>
> No.  I'm suggesting that the deduction logic should be much more 
> straightforward, just some sort of "is the type non-dependent and lacking a 
> qualifier", and it should be applied in the two basic places we build these 
> types in Sema, i.e. in the type-from-declarator logic and in the 
> `Build{Pointer,Reference}Type` logic.


Ok thanks, I made this change now.

> Instead we have something very elaborate that apparently recursively looks 
> through pointer types and is contingent on the exact spelling, e.g. trying to 
> find `auto` types, which seems both brittle and unnecessary.

I realized that I didn't have to do this actually. It was done by mistake 
earlier.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 218639.
Anastasia added a comment.

Moved addr space of pointee inference into Build* helpers of Sema.


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

https://reviews.llvm.org/D65744

Files:
  clang/include/clang/AST/Type.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -4535,14 +4535,6 @@
   return Result;
 }
 
-/// Helper to deduce addr space of a pointee type in OpenCL mode.
-/// If the type is updated it will be overwritten in PointeeType param.
-static void deduceOpenCLPointeeAddrSpace(Sema , QualType ) {
-  if (PointeeType.getAddressSpace() == LangAS::Default)
-PointeeType = SemaRef.Context.getAddrSpaceQualType(PointeeType,
-   LangAS::opencl_generic);
-}
-
 template
 QualType TreeTransform::TransformPointerType(TypeLocBuilder ,
   PointerTypeLoc TL) {
@@ -4551,9 +4543,6 @@
   if (PointeeType.isNull())
 return QualType();
 
-  if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
-
   QualType Result = TL.getType();
   if (PointeeType->getAs()) {
 // A dependent pointer type 'T *' has is being transformed such
@@ -4592,9 +4581,6 @@
   if (PointeeType.isNull())
 return QualType();
 
-  if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
-
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
   PointeeType != TL.getPointeeLoc().getType()) {
@@ -4624,9 +4610,6 @@
   if (PointeeType.isNull())
 return QualType();
 
-  if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
-
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
   PointeeType != T->getPointeeTypeAsWritten()) {
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1962,6 +1962,18 @@
   return true;
 }
 
+// Helper to deduce addr space of a pointee type in OpenCL mode.
+static QualType deduceOpenCLPointeeAddrSpace(Sema , QualType PointeeType) {
+  if (!PointeeType->isUndeducedAutoType() && !PointeeType->isDependentType() &&
+  !PointeeType.getQualifiers().hasAddressSpace())
+PointeeType = S.getASTContext().getAddrSpaceQualType(
+PointeeType,
+S.getLangOpts().OpenCLCPlusPlus || S.getLangOpts().OpenCLVersion == 200
+? LangAS::opencl_generic
+: LangAS::opencl_private);
+  return PointeeType;
+}
+
 /// Build a pointer type.
 ///
 /// \param T The type to which we'll be building a pointer.
@@ -1998,6 +2010,9 @@
   if (getLangOpts().ObjCAutoRefCount)
 T = inferARCLifetimeForPointee(*this, T, Loc, /*reference*/ false);
 
+  if (getLangOpts().OpenCL)
+T = deduceOpenCLPointeeAddrSpace(*this, T);
+
   // Build the pointer type.
   return Context.getPointerType(T);
 }
@@ -2058,6 +2073,9 @@
   if (getLangOpts().ObjCAutoRefCount)
 T = inferARCLifetimeForPointee(*this, T, Loc, /*reference*/ true);
 
+  if (getLangOpts().OpenCL)
+T = deduceOpenCLPointeeAddrSpace(*this, T);
+
   // Handle restrict on references.
   if (LValueRef)
 return Context.getLValueReferenceType(T, SpelledAsLValue);
@@ -2626,6 +2644,9 @@
   if (checkQualifiedFunction(*this, T, Loc, QFK_BlockPointer))
 return QualType();
 
+  if (getLangOpts().OpenCL)
+T = deduceOpenCLPointeeAddrSpace(*this, T);
+
   return Context.getBlockPointerType(T);
 }
 
@@ -7434,6 +7455,9 @@
   // Do not deduce addr space of decltype because it will be taken from
   // its argument.
   T->isDecltypeType() ||
+  // Do not deduce addr space for auto pointee type because it is taken from
+  // the initializing expression type during the type deduction.
+  (T->isUndeducedAutoType() && IsPointee) ||
   // OpenCL spec v2.0 s6.9.b:
   // The sampler type cannot be used with the __local and __global address
   // space qualifiers.
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1805,7 +1805,7 @@
 auto Ty = getType();
 auto SETy = getSubExpr()->getType();
 assert(getValueKindForType(Ty) == Expr::getValueKindForType(SETy));
-if (/*isRValue()*/ !Ty->getPointeeType().isNull()) {
+if (isRValue()) {
   Ty = Ty->getPointeeType();
   SETy = SETy->getPointeeType();
 }
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -2044,6 +2044,8 @@
   bool isAlignValT() const; 

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D65744#1652355 , @Anastasia wrote:

> I don't think this is likely to change. Are you suggesting to move the 
> deduction logic for pointee of pointers, references and block pointers into 
> ASTContext helper that creates a pointer/reference/block pointer type?


No.  I'm suggesting that the deduction logic should be much more 
straightforward, just some sort of "is the type non-dependent and lacking a 
qualifier", and it should be applied in the two basic places we build these 
types in Sema, i.e. in the type-from-declarator logic and in the 
`Build{Pointer,Reference}Type` logic.  Instead we have something very elaborate 
that apparently recursively looks through pointer types and is contingent on 
the exact spelling, e.g. trying to find `auto` types, which seems both brittle 
and unnecessary.




Comment at: include/clang/AST/Type.h:6509
+  return isa(CanonicalType);
+}
+

Hmm.  So this method, confusingly, will not return true for a deduced `auto`, 
unless the deduced type is itself an undeduced `auto` (which I'm not sure can 
happen).  I think it at least needs a different name; `isUndeducedAutoType()` 
would be okay if the latter case is not possible.  But it might be better if we 
can just define away the need for the method entirely.



Comment at: lib/Sema/SemaType.cpp:7441
+  // the initializing expression type during the type deduction.
+  (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
   // OpenCL spec v2.0 s6.9.b:

Anastasia wrote:
> mantognini wrote:
> > mantognini wrote:
> > > Shouldn't the parentheses around `IsAutoPointee` be removed for style 
> > > consistency?
> > With the `if` statement introduced above, `IsAutoPointee` can be true only 
> > in C++ mode. Could it be an issue to not guard `(T->isAutoType() && 
> > IsPointee)` for non-C++ mode? (I guess not, but I couldn't convince myself.)
> I think `TreeTransforms` will only be used in C++ mode. But also `isAutoType` 
> should only be true in C++ mode. So I think we should be fine.
I don't think `TreeTransform` is expected to be C++-only, but I agree that 
`isAutoType` is good enough.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked an inline comment as done.
Anastasia added a comment.

missing test case

  1 auto* accumulator()
  2 {
  3   __global int * glptr;
  4   return glptr;
  5 }
  6 int i;
  7 auto* ai = 
  8 int* ii = 

In D65744#1646348 , @rjmccall wrote:

> In D65744#1629055 , @Anastasia wrote:
>
> > In D65744#1627589 , @rjmccall 
> > wrote:
> >
> > > I see.  Is the deduction rule recursive or something, where a pointer to 
> > > pointer is inferred to point to the same address space as the pointee?
> >
> >
> > It is recursive indeed and we currently deduce all pointees to generic AS.
>
>
> Is that likely to change?  If all pointees are inferred to be generic (in the 
> absence of an explicit qualifier) when building a pointer type, this seems 
> very over-thought vs. just adding the generic AS whenever you build a pointer 
> to an unqualified but non-dependent type.


I don't think this is likely to change. Are you suggesting to move the 
deduction logic for pointee of pointers, references and block pointers into 
ASTContext helper that creates a pointer/reference/block pointer type? I guess 
it should work if the only way to construct the pointer is by using this 
helper. I can certainly prototype this change and see.




Comment at: lib/Sema/SemaType.cpp:7441
+  // the initializing expression type during the type deduction.
+  (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
   // OpenCL spec v2.0 s6.9.b:

mantognini wrote:
> mantognini wrote:
> > Shouldn't the parentheses around `IsAutoPointee` be removed for style 
> > consistency?
> With the `if` statement introduced above, `IsAutoPointee` can be true only in 
> C++ mode. Could it be an issue to not guard `(T->isAutoType() && IsPointee)` 
> for non-C++ mode? (I guess not, but I couldn't convince myself.)
I think `TreeTransforms` will only be used in C++ mode. But also `isAutoType` 
should only be true in C++ mode. So I think we should be fine.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D65744#1629055 , @Anastasia wrote:

> In D65744#1627589 , @rjmccall wrote:
>
> > I see.  Is the deduction rule recursive or something, where a pointer to 
> > pointer is inferred to point to the same address space as the pointee?
>
>
> It is recursive indeed and we currently deduce all pointees to generic AS.


Is that likely to change?  If all pointees are inferred to be generic (in the 
absence of an explicit qualifier) when building a pointer type, this seems very 
over-thought vs. just adding the generic AS whenever you build a pointer to an 
unqualified but non-dependent type.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

I think this looks good. Maybe the tests should be extended to test `auto` as 
function return type, and if there's some special handling around 
`decltype(auto)`, then it should be tested too, but I'm not sure it's actually 
needed here. What do you think?




Comment at: lib/Sema/SemaType.cpp:7441
+  // the initializing expression type during the type deduction.
+  (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
   // OpenCL spec v2.0 s6.9.b:

Shouldn't the parentheses around `IsAutoPointee` be removed for style 
consistency?



Comment at: lib/Sema/SemaType.cpp:7441
+  // the initializing expression type during the type deduction.
+  (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
   // OpenCL spec v2.0 s6.9.b:

mantognini wrote:
> Shouldn't the parentheses around `IsAutoPointee` be removed for style 
> consistency?
With the `if` statement introduced above, `IsAutoPointee` can be true only in 
C++ mode. Could it be an issue to not guard `(T->isAutoType() && IsPointee)` 
for non-C++ mode? (I guess not, but I couldn't convince myself.)



Comment at: lib/Sema/TreeTransform.h:4550
+Pointee = Pointee->getPointeeType();
+  }  while (!Pointee.isNull());
+  if (!IsAuto && PointeeType.getAddressSpace() == LangAS::Default)

Nitpicking: there are two spaces between `}` and `while`.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D65744#1627589 , @rjmccall wrote:

> I see.  Is the deduction rule recursive or something, where a pointer to 
> pointer is inferred to point to the same address space as the pointee?


It is recursive indeed and we currently deduce all pointees to generic AS.

> I still don't understand why pointers and references are handled differently 
> here, instead of having the rule be "don't infer if the type is opaquely 
> dependent or an `auto` placeholder".

Because we currently only infer pointers and references in TreeTransforms. 
Everything else is inferred earlier. That is mainly because we need some 
context info to infer in most of other cases and therefore it's not easy to do 
in TreeTransforms. There is some more information on this review: 
https://reviews.llvm.org/D64400 where the inference for pointers and references 
was added originally to TreeTransforms.


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I see.  Is the deduction rule recursive or something, where a pointer to 
pointer is inferred to point to the same address space as the pointee?  I still 
don't understand why pointers and references are handled differently here, 
instead of having the rule be "don't infer if the type is opaquely dependent or 
an `auto` placeholder".


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D65744#1626563 , @rjmccall wrote:

> Isn't the general rule for template argument deduction (which this devolves 
> to) just to ignore top-level qualifiers?  And then you can substitute in the 
> substituted type and end up with a properly qualified type for the parameter 
> / variable, and you can add extra qualifiers as necessary.  Why are special 
> rules for pointers and references required?


The issue I am trying to solve is that the addr space qualifier for the pointee 
type in pointers and references is supposed to default to generic if none were 
provided either in the parameter or argument of template/auto type. But for all 
regular types the deduction happens early during parsing.  So I need to prevent 
the early deduction  and make sure the deduction happens once the type is being 
known (i.e. deduced).


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Isn't the general rule for template argument deduction (which this devolves to) 
just to ignore top-level qualifiers?  And then you can substitute in the 
substituted type and end up with a properly qualified type for the parameter / 
variable, and you can add extra qualifiers as necessary.  Why are special rules 
for pointers and references required?


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

https://reviews.llvm.org/D65744



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: rjmccall.
Herald added subscribers: ebevhan, yaxunl.

A regression was introduced in D64400  because 
auto is using the same logic as templates. However, deduction of addr spaces 
wasn't working correctly even before.

Here are the rules that are implemented in this patch:

- For non-reference and non-pointer types deduction can be done early because 
addr space is not going to be taken from init expr
- For ref or ptr auto types we should prevent deducing addr space before the 
deduction of the whole type (incl its pointee addr spaces) from init expr


https://reviews.llvm.org/D65744

Files:
  include/clang/AST/Type.h
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  test/SemaOpenCLCXX/addrspace-auto.cl

Index: test/SemaOpenCLCXX/addrspace-auto.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/addrspace-auto.cl
@@ -0,0 +1,31 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+kernel void test(){
+  int i;
+//CHECK: VarDecl {{.*}} ai 'int':'int'
+  auto ai = i;
+
+  constexpr int c = 1;
+//CHECK: VarDecl {{.*}} used cai '__constant int':'__constant int'
+  __constant auto cai = c;
+//CHECK: VarDecl {{.*}} aii 'int':'int'
+  auto aii = cai;
+
+//CHECK: VarDecl {{.*}} ref 'int &'
+  auto& ref = i;
+//CHECK: VarDecl {{.*}} ptr 'int *'
+  auto* ptr = 
+//CHECK: VarDecl {{.*}} ref_c '__constant int &'
+  auto& ref_c = cai;
+
+//CHECK: VarDecl {{.*}} ptrptr 'int **'
+  auto ** ptrptr = 
+//CHECK: VarDecl {{.*}} refptr 'int *&'
+  auto *& refptr = ptr;
+
+//CHECK: VarDecl {{.*}} invalid gref '__global auto &'
+  __global auto& gref = i; //expected-error{{variable 'gref' with type '__global auto &' has incompatible initializer of type 'int'}}
+  __local int* ptr_l;
+//CHECK: VarDecl {{.*}} invalid gptr '__global auto *'
+  __global auto* gptr = ptr_l; //expected-error{{variable 'gptr' with type '__global auto *' has incompatible initializer of type '__local int *'}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -4538,8 +4538,17 @@
 
 /// Helper to deduce addr space of a pointee type in OpenCL mode.
 /// If the type is updated it will be overwritten in PointeeType param.
-static void deduceOpenCLPointeeAddrSpace(Sema , QualType ) {
-  if (PointeeType.getAddressSpace() == LangAS::Default)
+static void deduceOpenCLPointeeAddrSpace(Sema , QualType ,
+ QualType TLT) {
+  // Prevent deducing addr space for auto because it will be taken from
+  // the initializing expression.
+  bool IsAuto = false;
+  auto Pointee = TLT->getPointeeType();
+  do {
+IsAuto = Pointee->isAutoType();
+Pointee = Pointee->getPointeeType();
+  }  while (!Pointee.isNull());
+  if (!IsAuto && PointeeType.getAddressSpace() == LangAS::Default)
 PointeeType = SemaRef.Context.getAddrSpaceQualType(PointeeType,
LangAS::opencl_generic);
 }
@@ -4553,7 +4562,7 @@
 return QualType();
 
   if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
+deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType, TL.getType());
 
   QualType Result = TL.getType();
   if (PointeeType->getAs()) {
@@ -4594,7 +4603,7 @@
 return QualType();
 
   if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
+deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType, TL.getType());
 
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
@@ -4626,7 +4635,7 @@
 return QualType();
 
   if (SemaRef.getLangOpts().OpenCL)
-deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType);
+deduceOpenCLPointeeAddrSpace(SemaRef, PointeeType, TL.getType());
 
   QualType Result = TL.getType();
   if (getDerived().AlwaysRebuild() ||
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7398,6 +7398,17 @@
   bool IsFuncType =
   ChunkIndex < D.getNumTypeObjects() &&
   D.getTypeObject(ChunkIndex).Kind == DeclaratorChunk::Function;
+  bool IsAutoPointee = false;
+  if (State.getSema().getLangOpts().OpenCLCPlusPlus){
+// Detect if pointer or reference are part of auto type.
+if (T->isPointerType() || T->isReferenceType()) {
+  auto Pointee = T->getPointeeType();
+  do {
+IsAutoPointee = Pointee->isAutoType();
+Pointee = Pointee->getPointeeType();
+  } while (!Pointee.isNull());
+}
+  }
   if ( // Do not deduce addr space for function return type and function type,
// otherwise it will fail some sema check.
   IsFuncReturnType || IsFuncType ||
@@ -7425,6 +7436,9 @@
   // Do not deduce addr space of decltype because it will be taken from