[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366417: [OpenCL][PR42033] Fix addr space deduction with 
template parameters (authored by stulova, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62584?vs=210277=210502#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62584

Files:
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/lib/Sema/TreeTransform.h
  cfe/trunk/test/SemaOpenCLCXX/address-space-deduction.cl
  cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl


Index: cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
===
--- cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
+++ cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
@@ -3,7 +3,7 @@
 template 
 struct S {
   T a;// expected-error{{field may not be qualified with an address 
space}}
-  T f1(); // expected-error{{function type may not be qualified with an 
address space}}
+  T f1(); // we ignore address space on a return types.
   void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
 };
 
Index: cfe/trunk/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- cfe/trunk/test/SemaOpenCLCXX/address-space-deduction.cl
+++ cfe/trunk/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -63,3 +63,18 @@
 //CHECK: -CXXConstructorDecl {{.*}} x3 'void (const x3 &){{( 
__attribute__.*)?}} __generic'
 template 
 x3::x3(const x3 ) {}
+
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -7419,7 +7419,9 @@
   (T->isVoidType() && !IsPointee) ||
   // Do not deduce addr spaces for dependent types because they might end
   // up instantiating to a type with an explicit address space qualifier.
-  T->isDependentType() ||
+  // Except for pointer or reference types because the addr space in
+  // template argument can only belong to a pointee.
+  (T->isDependentType() && !T->isPointerType() && !T->isReferenceType()) ||
   // Do not deduce addr space of decltype because it will be taken from
   // its argument.
   T->isDecltypeType() ||
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -7491,7 +7491,10 @@
 return;
   }
 }
-  } else if (T.getAddressSpace() != LangAS::opencl_private) {
+  } else if (T.getAddressSpace() != LangAS::opencl_private &&
+ // If we are parsing a template we didn't deduce an addr
+ // space yet.
+ T.getAddressSpace() != LangAS::Default) {
 // Do not allow other address spaces on automatic variable.
 Diag(NewVD->getLocation(), diag::err_as_qualified_auto_decl) << 1;
 NewVD->setInvalidDecl();
Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -5392,13 +5392,6 @@
 if (ResultType.isNull())
   return QualType();
 
-// Return type can not be qualified with an address space.
-if (ResultType.getAddressSpace() != LangAS::Default) {
-  SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),
-   diag::err_attribute_address_function_type);
-  return QualType();
-}
-
 if (getDerived().TransformFunctionTypeParams(
 TL.getBeginLoc(), TL.getParams(),
 TL.getTypePtr()->param_type_begin(),


Index: cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
===
--- cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
+++ cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
@@ -3,7 +3,7 @@
 template 
 struct S {
   T a;// expected-error{{field may not be qualified with an address space}}
-  T f1(); // expected-error{{function type may not be qualified with an address space}}
+  T f1(); // we ignore address space on a return types.
   void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
Index: cfe/trunk/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- 

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

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

Thanks!


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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

Fixed typo in the comment


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

https://reviews.llvm.org/D62584

Files:
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  test/SemaOpenCLCXX/address-space-deduction.cl
  test/SemaOpenCLCXX/address-space-templates.cl


Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -3,7 +3,7 @@
 template 
 struct S {
   T a;// expected-error{{field may not be qualified with an address 
space}}
-  T f1(); // expected-error{{function type may not be qualified with an 
address space}}
+  T f1(); // we ignore address space on a return types.
   void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
 };
 
Index: test/SemaOpenCLCXX/address-space-deduction.cl
===
--- test/SemaOpenCLCXX/address-space-deduction.cl
+++ test/SemaOpenCLCXX/address-space-deduction.cl
@@ -24,3 +24,17 @@
   alias_c1_ptr ptr = 
 };
 
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -5365,13 +5365,6 @@
 if (ResultType.isNull())
   return QualType();
 
-// Return type can not be qualified with an address space.
-if (ResultType.getAddressSpace() != LangAS::Default) {
-  SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),
-   diag::err_attribute_address_function_type);
-  return QualType();
-}
-
 if (getDerived().TransformFunctionTypeParams(
 TL.getBeginLoc(), TL.getParams(),
 TL.getTypePtr()->param_type_begin(),
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7414,7 +7414,9 @@
   (T->isVoidType() && !IsPointee) ||
   // Do not deduce addr spaces for dependent types because they might end
   // up instantiating to a type with an explicit address space qualifier.
-  T->isDependentType() ||
+  // Except for pointer or reference types because the addr space in
+  // template argument can only belong to a pointee.
+  (T->isDependentType() && !T->isPointerType() && !T->isReferenceType()) ||
   // Do not deduce addr space of decltype because it will be taken from
   // its argument.
   T->isDecltypeType() ||
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -7484,7 +7484,10 @@
 return;
   }
 }
-  } else if (T.getAddressSpace() != LangAS::opencl_private) {
+  } else if (T.getAddressSpace() != LangAS::opencl_private &&
+ // If we are parsing a template we didn't deduce an addr
+ // space yet.
+ T.getAddressSpace() != LangAS::Default) {
 // Do not allow other address spaces on automatic variable.
 Diag(NewVD->getLocation(), diag::err_as_qualified_auto_decl) << 1;
 NewVD->setInvalidDecl();


Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -3,7 +3,7 @@
 template 
 struct S {
   T a;// expected-error{{field may not be qualified with an address space}}
-  T f1(); // expected-error{{function type may not be qualified with an address space}}
+  T f1(); // we ignore address space on a return types.
   void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
Index: test/SemaOpenCLCXX/address-space-deduction.cl
===
--- test/SemaOpenCLCXX/address-space-deduction.cl
+++ test/SemaOpenCLCXX/address-space-deduction.cl
@@ -24,3 +24,17 @@
   alias_c1_ptr ptr = 
 };
 
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -5365,13 +5365,6 @@
 if (ResultType.isNull())

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Minor comment then LGTM




Comment at: lib/Sema/SemaType.cpp:7418
+  // Expect for pointer or reference types because the addr space in
+  // template argument can only belong to a pointee.
+  (T->isDependentType() && !T->isPointerType() && !T->isReferenceType()) ||

"Except"


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62584#1586640 , @rjmccall wrote:

> In D62584#1585091 , @Anastasia wrote:
>
> > In D62584#1583340 , @rjmccall 
> > wrote:
> >
> > > Oh, yes, it definitely can't be done to class types.  I suppose we should 
> > > just forget about it.
> >
> >
> > Ok, regarding address space qualifiers in template instantiation - is it 
> > still ok that we ignore them here in this patch?
>
>
> I think ignoring them in templates rather than diagnosing is the right thing 
> to do, since it means that reasonable things, e.g. template functions 
> returning `T`, won't be erroneous if you instantiate `T = __global int` just 
> like they wouldn't be with `T = const int`.


Ok cool, I have removed the diagnostic btw. :)


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D62584#1585091 , @Anastasia wrote:

> In D62584#1583340 , @rjmccall wrote:
>
> > Oh, yes, it definitely can't be done to class types.  I suppose we should 
> > just forget about it.
>
>
> Ok, regarding address space qualifiers in template instantiation - is it 
> still ok that we ignore them here in this patch?


I think ignoring them in templates rather than diagnosing is the right thing to 
do, since it means that reasonable things, e.g. template functions returning 
`T`, won't be erroneous if you instantiate `T = __global int` just like they 
wouldn't be with `T = const int`.


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62584#1583340 , @rjmccall wrote:

> Oh, yes, it definitely can't be done to class types.  I suppose we should 
> just forget about it.


Ok, regarding address space qualifiers in template instantiation - is it still 
ok that we ignore them here in this patch?


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Oh, yes, it definitely can't be done to class types.  I suppose we should just 
forget about it.


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > Anastasia wrote:
> > > > > rjmccall wrote:
> > > > > > Anastasia wrote:
> > > > > > > I am trying to be a bit more helpful here although I am not sure 
> > > > > > > if we should instead require explicit template parameter and fail 
> > > > > > > the template deduction instead.
> > > > > > > 
> > > > > > > Basically, do we want the following code to always require 
> > > > > > > specifying template argument explicitly:
> > > > > > > 
> > > > > > > 
> > > > > > > ```
> > > > > > > template 
> > > > > > > T xxx(T *in) {
> > > > > > >   T *i = in;
> > > > > > >   return *i;
> > > > > > > }
> > > > > > > 
> > > > > > > __kernel void test() {
> > > > > > >   int foo[10];
> > > > > > >   xxx([0]); // if we deduce type from foo, it ends up 
> > > > > > > being qualified by __private that we currently diagnose. However 
> > > > > > > private is default (implicit) address space for return type so 
> > > > > > > technically there is no danger in just allowing xxx([0])
> > > > > > > }
> > > > > > > ```
> > > > > > Implicitly ignoring all address-space qualifiers on the return type 
> > > > > > seems like the right thing to do; I don't think it needs to be 
> > > > > > limited to `__private`.  That's probably also the right thing to do 
> > > > > > for locals, but I'm less certain about it.
> > > > > Just to clarify by "Implicitly ignoring" you mean ignoring if the 
> > > > > templ parameters were deduced?
> > > > > 
> > > > > Although I am a bit concerned about allowing other than `__private` 
> > > > > address spaces in return types as we reject them in return types of 
> > > > > functions generally. Would it not be somehow inconsistent?
> > > > Ok, I have removed the diagnostic completely. At least we don't seem to 
> > > > generate any incorrect IR.
> > > They should be diagnosed somehow when written explicitly on a return 
> > > type, but if you just do that on the parser path you don't have to worry 
> > > about it during template instantiation.  They should probably otherwise 
> > > be ignored no matter where they came from — if someone typedefs 
> > > `private_int_t` to `__private int`, you should just treat that as `int` 
> > > in a return type.  Stripping the qualifier from the type is probably the 
> > > right thing to do so that it doesn't further impact semantic analysis.
> > > 
> > > I definitely don't think you want a model where the qualifier actually 
> > > means that the return is somehow done via an object in that address space.
> > > They should be diagnosed somehow when written explicitly on a return 
> > > type, but if you just do that on the parser path you don't have to worry 
> > > about it during template instantiation.
> > 
> > Ok, this seems to be working currently. The following won't compile:
> > 
> > ```
> > template 
> > struct S {
> >   __global T f1(); // error: return value cannot be qualified with 
> > address space
> > };
> > 
> > ```
> > 
> > > They should probably otherwise be ignored no matter where they came from 
> > > — if someone typedefs private_int_t to __private int, you should just 
> > > treat that as int in a return type.
> > 
> > We produce diagnostic for this case currently. I can update this in a 
> > separate patch if you think it's more helpful behavior. Although I feel a 
> > bit worried about it. However it would align with what we are doing with 
> > templates here... so perhaps it's logical change... 
> > 
> > > Stripping the qualifier from the type is probably the right thing to do 
> > > so that it doesn't further impact semantic analysis.
> > 
> > I guess you mean stripping quals for the case of typedef or others where we 
> > will accept the code and ignore quals on the return type? I have tried to 
> > do this just for the template case but there are some assertions firing 
> > because we are expecting the original return type to be the same before and 
> > after template instantiation. So it feels like I would have to do it for 
> > everything together. Maybe it should rather go into separate review?
> > We produce diagnostic for this case currently. I can update this in a 
> > separate patch if you think it's more helpful behavior. Although I feel a 
> > bit worried about it. However it would align with what we are doing with 
> > templates here... so perhaps it's logical change...
> 
> Well, it's debatable.  C doesn't say that qualifiers are erased in function 
> return types in general.  On the other hand, qualifiers are semantically 
> meaningless there, and C has few rules that rely on exact type identity.  On 
> reflection, we 

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > Anastasia wrote:
> > > > > > I am trying to be a bit more helpful here although I am not sure if 
> > > > > > we should instead require explicit template parameter and fail the 
> > > > > > template deduction instead.
> > > > > > 
> > > > > > Basically, do we want the following code to always require 
> > > > > > specifying template argument explicitly:
> > > > > > 
> > > > > > 
> > > > > > ```
> > > > > > template 
> > > > > > T xxx(T *in) {
> > > > > >   T *i = in;
> > > > > >   return *i;
> > > > > > }
> > > > > > 
> > > > > > __kernel void test() {
> > > > > >   int foo[10];
> > > > > >   xxx([0]); // if we deduce type from foo, it ends up 
> > > > > > being qualified by __private that we currently diagnose. However 
> > > > > > private is default (implicit) address space for return type so 
> > > > > > technically there is no danger in just allowing xxx([0])
> > > > > > }
> > > > > > ```
> > > > > Implicitly ignoring all address-space qualifiers on the return type 
> > > > > seems like the right thing to do; I don't think it needs to be 
> > > > > limited to `__private`.  That's probably also the right thing to do 
> > > > > for locals, but I'm less certain about it.
> > > > Just to clarify by "Implicitly ignoring" you mean ignoring if the templ 
> > > > parameters were deduced?
> > > > 
> > > > Although I am a bit concerned about allowing other than `__private` 
> > > > address spaces in return types as we reject them in return types of 
> > > > functions generally. Would it not be somehow inconsistent?
> > > Ok, I have removed the diagnostic completely. At least we don't seem to 
> > > generate any incorrect IR.
> > They should be diagnosed somehow when written explicitly on a return type, 
> > but if you just do that on the parser path you don't have to worry about it 
> > during template instantiation.  They should probably otherwise be ignored 
> > no matter where they came from — if someone typedefs `private_int_t` to 
> > `__private int`, you should just treat that as `int` in a return type.  
> > Stripping the qualifier from the type is probably the right thing to do so 
> > that it doesn't further impact semantic analysis.
> > 
> > I definitely don't think you want a model where the qualifier actually 
> > means that the return is somehow done via an object in that address space.
> > They should be diagnosed somehow when written explicitly on a return type, 
> > but if you just do that on the parser path you don't have to worry about it 
> > during template instantiation.
> 
> Ok, this seems to be working currently. The following won't compile:
> 
> ```
> template 
> struct S {
>   __global T f1(); // error: return value cannot be qualified with 
> address space
> };
> 
> ```
> 
> > They should probably otherwise be ignored no matter where they came from — 
> > if someone typedefs private_int_t to __private int, you should just treat 
> > that as int in a return type.
> 
> We produce diagnostic for this case currently. I can update this in a 
> separate patch if you think it's more helpful behavior. Although I feel a bit 
> worried about it. However it would align with what we are doing with 
> templates here... so perhaps it's logical change... 
> 
> > Stripping the qualifier from the type is probably the right thing to do so 
> > that it doesn't further impact semantic analysis.
> 
> I guess you mean stripping quals for the case of typedef or others where we 
> will accept the code and ignore quals on the return type? I have tried to do 
> this just for the template case but there are some assertions firing because 
> we are expecting the original return type to be the same before and after 
> template instantiation. So it feels like I would have to do it for everything 
> together. Maybe it should rather go into separate review?
> We produce diagnostic for this case currently. I can update this in a 
> separate patch if you think it's more helpful behavior. Although I feel a bit 
> worried about it. However it would align with what we are doing with 
> templates here... so perhaps it's logical change...

Well, it's debatable.  C doesn't say that qualifiers are erased in function 
return types in general.  On the other hand, qualifiers are semantically 
meaningless there, and C has few rules that rely on exact type identity.  On 
reflection, we should probably keep diagnosing that at parse-time, even for 
typedefs, and just ignore it in templates.

Can you point out which assertion is firing?


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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

rjmccall wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Anastasia wrote:
> > > > > I am trying to be a bit more helpful here although I am not sure if 
> > > > > we should instead require explicit template parameter and fail the 
> > > > > template deduction instead.
> > > > > 
> > > > > Basically, do we want the following code to always require specifying 
> > > > > template argument explicitly:
> > > > > 
> > > > > 
> > > > > ```
> > > > > template 
> > > > > T xxx(T *in) {
> > > > >   T *i = in;
> > > > >   return *i;
> > > > > }
> > > > > 
> > > > > __kernel void test() {
> > > > >   int foo[10];
> > > > >   xxx([0]); // if we deduce type from foo, it ends up being 
> > > > > qualified by __private that we currently diagnose. However private is 
> > > > > default (implicit) address space for return type so technically there 
> > > > > is no danger in just allowing xxx([0])
> > > > > }
> > > > > ```
> > > > Implicitly ignoring all address-space qualifiers on the return type 
> > > > seems like the right thing to do; I don't think it needs to be limited 
> > > > to `__private`.  That's probably also the right thing to do for locals, 
> > > > but I'm less certain about it.
> > > Just to clarify by "Implicitly ignoring" you mean ignoring if the templ 
> > > parameters were deduced?
> > > 
> > > Although I am a bit concerned about allowing other than `__private` 
> > > address spaces in return types as we reject them in return types of 
> > > functions generally. Would it not be somehow inconsistent?
> > Ok, I have removed the diagnostic completely. At least we don't seem to 
> > generate any incorrect IR.
> They should be diagnosed somehow when written explicitly on a return type, 
> but if you just do that on the parser path you don't have to worry about it 
> during template instantiation.  They should probably otherwise be ignored no 
> matter where they came from — if someone typedefs `private_int_t` to 
> `__private int`, you should just treat that as `int` in a return type.  
> Stripping the qualifier from the type is probably the right thing to do so 
> that it doesn't further impact semantic analysis.
> 
> I definitely don't think you want a model where the qualifier actually means 
> that the return is somehow done via an object in that address space.
> They should be diagnosed somehow when written explicitly on a return type, 
> but if you just do that on the parser path you don't have to worry about it 
> during template instantiation.

Ok, this seems to be working currently. The following won't compile:

```
template 
struct S {
  __global T f1(); // error: return value cannot be qualified with address 
space
};

```

> They should probably otherwise be ignored no matter where they came from — if 
> someone typedefs private_int_t to __private int, you should just treat that 
> as int in a return type.

We produce diagnostic for this case currently. I can update this in a separate 
patch if you think it's more helpful behavior. Although I feel a bit worried 
about it. However it would align with what we are doing with templates here... 
so perhaps it's logical change... 

> Stripping the qualifier from the type is probably the right thing to do so 
> that it doesn't further impact semantic analysis.

I guess you mean stripping quals for the case of typedef or others where we 
will accept the code and ignore quals on the return type? I have tried to do 
this just for the template case but there are some assertions firing because we 
are expecting the original return type to be the same before and after template 
instantiation. So it feels like I would have to do it for everything together. 
Maybe it should rather go into separate review?


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

Anastasia wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > I am trying to be a bit more helpful here although I am not sure if we 
> > > > should instead require explicit template parameter and fail the 
> > > > template deduction instead.
> > > > 
> > > > Basically, do we want the following code to always require specifying 
> > > > template argument explicitly:
> > > > 
> > > > 
> > > > ```
> > > > template 
> > > > T xxx(T *in) {
> > > >   T *i = in;
> > > >   return *i;
> > > > }
> > > > 
> > > > __kernel void test() {
> > > >   int foo[10];
> > > >   xxx([0]); // if we deduce type from foo, it ends up being 
> > > > qualified by __private that we currently diagnose. However private is 
> > > > default (implicit) address space for return type so technically there 
> > > > is no danger in just allowing xxx([0])
> > > > }
> > > > ```
> > > Implicitly ignoring all address-space qualifiers on the return type seems 
> > > like the right thing to do; I don't think it needs to be limited to 
> > > `__private`.  That's probably also the right thing to do for locals, but 
> > > I'm less certain about it.
> > Just to clarify by "Implicitly ignoring" you mean ignoring if the templ 
> > parameters were deduced?
> > 
> > Although I am a bit concerned about allowing other than `__private` address 
> > spaces in return types as we reject them in return types of functions 
> > generally. Would it not be somehow inconsistent?
> Ok, I have removed the diagnostic completely. At least we don't seem to 
> generate any incorrect IR.
They should be diagnosed somehow when written explicitly on a return type, but 
if you just do that on the parser path you don't have to worry about it during 
template instantiation.  They should probably otherwise be ignored no matter 
where they came from — if someone typedefs `private_int_t` to `__private int`, 
you should just treat that as `int` in a return type.  Stripping the qualifier 
from the type is probably the right thing to do so that it doesn't further 
impact semantic analysis.

I definitely don't think you want a model where the qualifier actually means 
that the return is somehow done via an object in that address space.


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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

- Removed diagnostic for address space in return type in tree transforms


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

https://reviews.llvm.org/D62584

Files:
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  test/SemaOpenCLCXX/address-space-deduction.cl
  test/SemaOpenCLCXX/address-space-templates.cl


Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -3,7 +3,7 @@
 template 
 struct S {
   T a;// expected-error{{field may not be qualified with an address 
space}}
-  T f1(); // expected-error{{function type may not be qualified with an 
address space}}
+  T f1(); // we ignore address space on a return types.
   void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
 };
 
Index: test/SemaOpenCLCXX/address-space-deduction.cl
===
--- test/SemaOpenCLCXX/address-space-deduction.cl
+++ test/SemaOpenCLCXX/address-space-deduction.cl
@@ -24,3 +24,17 @@
   alias_c1_ptr ptr = 
 };
 
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -5365,13 +5365,6 @@
 if (ResultType.isNull())
   return QualType();
 
-// Return type can not be qualified with an address space.
-if (ResultType.getAddressSpace() != LangAS::Default) {
-  SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),
-   diag::err_attribute_address_function_type);
-  return QualType();
-}
-
 if (getDerived().TransformFunctionTypeParams(
 TL.getBeginLoc(), TL.getParams(),
 TL.getTypePtr()->param_type_begin(),
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7414,7 +7414,9 @@
   (T->isVoidType() && !IsPointee) ||
   // Do not deduce addr spaces for dependent types because they might end
   // up instantiating to a type with an explicit address space qualifier.
-  T->isDependentType() ||
+  // Expect for pointer or reference types because the addr space in
+  // template argument can only belong to a pointee.
+  (T->isDependentType() && !T->isPointerType() && !T->isReferenceType()) ||
   // Do not deduce addr space of decltype because it will be taken from
   // its argument.
   T->isDecltypeType() ||
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -7484,7 +7484,10 @@
 return;
   }
 }
-  } else if (T.getAddressSpace() != LangAS::opencl_private) {
+  } else if (T.getAddressSpace() != LangAS::opencl_private &&
+ // If we are parsing a template we didn't deduce an addr
+ // space yet.
+ T.getAddressSpace() != LangAS::Default) {
 // Do not allow other address spaces on automatic variable.
 Diag(NewVD->getLocation(), diag::err_as_qualified_auto_decl) << 1;
 NewVD->setInvalidDecl();


Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -3,7 +3,7 @@
 template 
 struct S {
   T a;// expected-error{{field may not be qualified with an address space}}
-  T f1(); // expected-error{{function type may not be qualified with an address space}}
+  T f1(); // we ignore address space on a return types.
   void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
Index: test/SemaOpenCLCXX/address-space-deduction.cl
===
--- test/SemaOpenCLCXX/address-space-deduction.cl
+++ test/SemaOpenCLCXX/address-space-deduction.cl
@@ -24,3 +24,17 @@
   alias_c1_ptr ptr = 
 };
 
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ 

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > I am trying to be a bit more helpful here although I am not sure if we 
> > > should instead require explicit template parameter and fail the template 
> > > deduction instead.
> > > 
> > > Basically, do we want the following code to always require specifying 
> > > template argument explicitly:
> > > 
> > > 
> > > ```
> > > template 
> > > T xxx(T *in) {
> > >   T *i = in;
> > >   return *i;
> > > }
> > > 
> > > __kernel void test() {
> > >   int foo[10];
> > >   xxx([0]); // if we deduce type from foo, it ends up being 
> > > qualified by __private that we currently diagnose. However private is 
> > > default (implicit) address space for return type so technically there is 
> > > no danger in just allowing xxx([0])
> > > }
> > > ```
> > Implicitly ignoring all address-space qualifiers on the return type seems 
> > like the right thing to do; I don't think it needs to be limited to 
> > `__private`.  That's probably also the right thing to do for locals, but 
> > I'm less certain about it.
> Just to clarify by "Implicitly ignoring" you mean ignoring if the templ 
> parameters were deduced?
> 
> Although I am a bit concerned about allowing other than `__private` address 
> spaces in return types as we reject them in return types of functions 
> generally. Would it not be somehow inconsistent?
Ok, I have removed the diagnostic completely. At least we don't seem to 
generate any incorrect IR.


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

rjmccall wrote:
> Anastasia wrote:
> > I am trying to be a bit more helpful here although I am not sure if we 
> > should instead require explicit template parameter and fail the template 
> > deduction instead.
> > 
> > Basically, do we want the following code to always require specifying 
> > template argument explicitly:
> > 
> > 
> > ```
> > template 
> > T xxx(T *in) {
> >   T *i = in;
> >   return *i;
> > }
> > 
> > __kernel void test() {
> >   int foo[10];
> >   xxx([0]); // if we deduce type from foo, it ends up being 
> > qualified by __private that we currently diagnose. However private is 
> > default (implicit) address space for return type so technically there is no 
> > danger in just allowing xxx([0])
> > }
> > ```
> Implicitly ignoring all address-space qualifiers on the return type seems 
> like the right thing to do; I don't think it needs to be limited to 
> `__private`.  That's probably also the right thing to do for locals, but I'm 
> less certain about it.
Just to clarify by "Implicitly ignoring" you mean ignoring if the templ 
parameters were deduced?

Although I am a bit concerned about allowing other than `__private` address 
spaces in return types as we reject them in return types of functions 
generally. Would it not be somehow inconsistent?


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

Anastasia wrote:
> I am trying to be a bit more helpful here although I am not sure if we should 
> instead require explicit template parameter and fail the template deduction 
> instead.
> 
> Basically, do we want the following code to always require specifying 
> template argument explicitly:
> 
> 
> ```
> template 
> T xxx(T *in) {
>   T *i = in;
>   return *i;
> }
> 
> __kernel void test() {
>   int foo[10];
>   xxx([0]); // if we deduce type from foo, it ends up being 
> qualified by __private that we currently diagnose. However private is default 
> (implicit) address space for return type so technically there is no danger in 
> just allowing xxx([0])
> }
> ```
Implicitly ignoring all address-space qualifiers on the return type seems like 
the right thing to do; I don't think it needs to be limited to `__private`.  
That's probably also the right thing to do for locals, but I'm less certain 
about it.


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/TreeTransform.h:5363
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

I am trying to be a bit more helpful here although I am not sure if we should 
instead require explicit template parameter and fail the template deduction 
instead.

Basically, do we want the following code to always require specifying template 
argument explicitly:


```
template 
T xxx(T *in) {
  T *i = in;
  return *i;
}

__kernel void test() {
  int foo[10];
  xxx([0]); // if we deduce type from foo, it ends up being qualified 
by __private that we currently diagnose. However private is default (implicit) 
address space for return type so technically there is no danger in just 
allowing xxx([0])
}
```


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62584#1522438 , @rjmccall wrote:

> I think the right approach here is probably to make sure you're applying 
> deduction during instantiation as well.


I agree I think we might need to extend the template instantiation logic to 
account for some corner cases. However, all local variables in OpenCL are to be 
deduced to `__private`, therefore would it be better to deduce them already as 
we parse the template definition instead of doing it multiple times on each 
instantiation? It doesn't seem the template argument should affect this rule...


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

https://reviews.llvm.org/D62584



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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 202753.
Anastasia retitled this revision from "[OpenCL][PR42033] Deducing addr space of 
pointer/reference with template parameter types" to "[OpenCL][PR42033] Deducing 
addr space with template parameter types".
Anastasia edited the summary of this revision.
Anastasia added a comment.

- Added more test cases
- Improved diagnostics to account for use of templates with addr spaces


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

https://reviews.llvm.org/D62584

Files:
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  test/SemaOpenCLCXX/address-space-deduction.cl


Index: test/SemaOpenCLCXX/address-space-deduction.cl
===
--- test/SemaOpenCLCXX/address-space-deduction.cl
+++ test/SemaOpenCLCXX/address-space-deduction.cl
@@ -10,3 +10,18 @@
   //CHECK: `-VarDecl {{.*}} foo2 'const __global int' static constexpr cinit
   static constexpr int foo2 = 0;
 };
+
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -5355,8 +5355,12 @@
 if (ResultType.isNull())
   return QualType();
 
-// Return type can not be qualified with an address space.
-if (ResultType.getAddressSpace() != LangAS::Default) {
+// Return type can not be qualified with an address space apart from when 
it
+// comes from a template argument in which case we can accept OpenCL 
private
+// address space because similar to Default it is used for an automatic
+// storage.
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace() != LangAS::opencl_private)) {
   SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),
diag::err_attribute_address_function_type);
   return QualType();
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7360,7 +7360,9 @@
   (T->isVoidType() && !IsPointee) ||
   // Do not deduce addr spaces for dependent types because they might end
   // up instantiating to a type with an explicit address space qualifier.
-  T->isDependentType() ||
+  // Expect for pointer or reference types because the addr space in
+  // template argument can only belong to a pointee.
+  (T->isDependentType() && !T->isPointerType() && !T->isReferenceType()) ||
   // Do not deduce addr space of decltype because it will be taken from
   // its argument.
   T->isDecltypeType() ||
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -7476,7 +7476,10 @@
 return;
   }
 }
-  } else if (T.getAddressSpace() != LangAS::opencl_private) {
+  } else if (T.getAddressSpace() != LangAS::opencl_private &&
+ // If we are parsing a template we didn't deduce an addr
+ // space yet.
+ T.getAddressSpace() != LangAS::Default) {
 // Do not allow other address spaces on automatic variable.
 Diag(NewVD->getLocation(), diag::err_as_qualified_auto_decl) << 1;
 NewVD->setInvalidDecl();


Index: test/SemaOpenCLCXX/address-space-deduction.cl
===
--- test/SemaOpenCLCXX/address-space-deduction.cl
+++ test/SemaOpenCLCXX/address-space-deduction.cl
@@ -10,3 +10,18 @@
   //CHECK: `-VarDecl {{.*}} foo2 'const __global int' static constexpr cinit
   static constexpr int foo2 = 0;
 };
+
+template 
+T xxx(T *in) {
+  // 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 ii;
+  return *i;
+}
+
+__kernel void test() {
+  int foo[10];
+  xxx([0]);
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -5355,8 +5355,12 @@
 if (ResultType.isNull())
   return QualType();
 
-// Return type can not be qualified with an address space.
-if (ResultType.getAddressSpace() != LangAS::Default) {
+// Return type can not be qualified with an address space apart from when it
+// comes from a template argument in which case we can accept OpenCL private
+// address space because similar to Default it is used for an automatic
+// storage.
+if (ResultType.getAddressSpace() != LangAS::Default &&
+(ResultType.getAddressSpace()