[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D55850#1369008 , @ebevhan wrote:

> I think the code that comes to mind is mostly like in 
> `GetTypeSourceInfoForDeclarator`:
>
>   LangAS AS =
>   (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);
>   
>
> It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if 
> it's not OpenCL++, but there shouldn't be a reason why the rest of the code 
> in that block won't work for regular C++.
>
> In fact, in most regular C++ this would be an issue, since Default typically 
> _is_ the 'generic' address space there.


Yes, `opencl_generic` is something quite specific to OpenCL I feel. I am not 
sure we can generalize it to C++.  I would quite like to understand this 
better. However, I think we could potentially implement `opencl_generic` using 
`Default` if only earlier OpenCL implementations wouldn't use `Default` for 
`opencl_private`. And because everything was built on top of this assumption 
it's now a big and a scary refactoring change to make. However, I think we can 
align different language modes better if `opencl_generic` could be implemented 
as `Default`. I might look into this at some point.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:5151
 
+  if (FromTypeCanon.getQualifiers().hasAddressSpace()) {
+Qualifiers QualsImplicitParamType = ImplicitParamType.getQualifiers();

I tested this patch with some kludges to let it parse our address space 
qualifiers, and hit a problem here; just because the FromType isn't qualified 
doesn't mean that the object conversion is okay. A conversion of an object 
`Type` to `__X Type` might not be legal if there is no conversion from 'no 
address space' to '__X' address space.

The example was a class with an AS-qualified overload, and when resolving for 
calling the overloaded method on an object `Type *`, it hit an ambiguous 
resolution since it considered the AS-qualified method to be a legal candidate.

It feels like this might be rather specific to how a language/target wants to 
use address spaces, though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think the code that comes to mind is mostly like in 
`GetTypeSourceInfoForDeclarator`:

  LangAS AS =
  (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);

It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if 
it's not OpenCL++, but there shouldn't be a reason why the rest of the code in 
that block won't work for regular C++.

In fact, in most regular C++ this would be an issue, since Default typically 
_is_ the 'generic' address space there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55850#1368767 , @Anastasia wrote:

> In D55850#1366709 , @rjmccall wrote:
>
> > Most of the remaining OpenCL checks are for OpenCL-specific logic like 
> > inferring the default address space, and yeah, we could probably make that 
> > a target option or something.
>
>
> We have an address space map in the targets that can be used to map default 
> address space if needed. Or are you suggesting to migrate functionality like 
> `deduceOpenCLImplicitAddrSpace()` to the target setting? Although I feel this 
> belong to the language rather.


True.  But we could make it a Sema-wide setting, configured immediately so that 
we don't have to repeatedly check a language option whenever we parse or 
synthesize a method.  It would make that logic feel less language-specific.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D55850#1366709 , @rjmccall wrote:

> Most of the remaining OpenCL checks are for OpenCL-specific logic like 
> inferring the default address space, and yeah, we could probably make that a 
> target option or something.


We have an address space map in the targets that can be used to map default 
address space if needed. Or are you suggesting to migrate functionality like 
`deduceOpenCLImplicitAddrSpace()` to the target setting? Although I feel this 
belong to the language rather.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I'll just add that we've been trying to not put things behind OpenCL guards as 
much as possible.  Most of the remaining OpenCL checks are for OpenCL-specific 
logic like inferring the default address space, and yeah, we could probably 
make that a target option or something.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D55850#1366094 , @ebevhan wrote:

> Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the 
> patches when they're up. Haven't done much serious testing on my end so far, 
> but from what I've seen the patches so far look good!


Cool! Thanks a lot for your feedback btw. Let me know if you discover any 
issues/bugs! I do need to improve testing at some point soon as I am sure there 
are still a lot of uncaught corner cases.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the 
patches when they're up. Haven't done much serious testing on my end so far, 
but from what I've seen the patches so far look good!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D55850#1365437 , @ebevhan wrote:

> I'm a bit late to the party here, it was an older patch so I wasn't watching 
> this one.
>
> I get a bit miffed when address space related features get locked behind 
> langoptions that aren't strictly address spaces. I know that there are 
> currently a couple code snippets which are behind LangOptions.OpenCL, that 
> are needed for address spaces to work reasonably even when you aren't using 
> OpenCL.
>
> I guess I do understand that the only address spaces that are interesting to 
> parse here are the named qualifier ones, but it would be convenient if the 
> parsing would accept other ones as well (such as the `__attribute__` based 
> ones) and not necessarily assume that the user is using OpenCL++.
>
> EDIT: Sort of jumped the gun there... The summary even says "This patch 
> doesn't enable the feature for C++ yet but it can easily be generalized if 
> the overall flow is right." Don't mind me.


This is correct. My plan was to generalize to C++ in the next patch. As I am 
more familiar with OpenCL and many rules are already well defined, this was my 
starting point. However, I want this work to be used for C++ based 
implementations generically and not just for OpenCL. I am still struggling to 
understand the semantic of address spaces in C++ for some cases and need a 
little bit more time, but I think I should be able to upload the initial patch 
soon. I would then appreciate any help with reviewing to get it right!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I'm a bit late to the party here, it was an older patch so I wasn't watching 
this one.

I get a bit miffed when address space related features get locked behind 
langoptions that aren't strictly address spaces. I know that there are 
currently a couple code snippets which are behind LangOptions.OpenCL, that are 
needed for address spaces to work reasonably even when you aren't using OpenCL.

I guess I do understand that the only address spaces that are interesting to 
parse here are the named qualifier ones, but it would be convenient if the 
parsing would accept other ones as well (such as the `__attribute__` based 
ones) and not necessarily assume that the user is using OpenCL++.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

After rebase I had to modify the following test:

  Index: test/SemaOpenCLCXX/address_space_overloading.cl
  ===
  --- test/SemaOpenCLCXX/address_space_overloading.cl   (revision 351746)
  +++ test/SemaOpenCLCXX/address_space_overloading.cl   (working copy)
  @@ -1,12 +1,12 @@
   // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
   
  -// expected-no-diagnostics
  +// FIXME: This test shouldn't trigger any errors.
   
   struct RetGlob {
 int dummy;
   };
   
  -struct RetGen {
  +struct RetGen { //expected-error{{binding value of type '__generic RetGen' 
to reference to type 'RetGen' drops <> qualifiers}}
 char dummy;
   };
   
  @@ -19,5 +19,5 @@
 __local int *ArgLoc;
 RetGlob TestGlob = foo(ArgGlob);
 RetGen TestGen = foo(ArgGen);
  -  TestGen = foo(ArgLoc);
  +  TestGen = foo(ArgLoc); //expected-note{{in implicit copy assignment 
operator for 'RetGen' first required here}}
   }

After looking at it, I realized that there is another un-handled path for 
address spaces in the initialization sequence. That gets hit during the 
creation of a return statement when we define implicit copy assignment. 
assignment. As a result I end up with the following incorrect AST:

  UnaryOperator 0x75a910 '__generic struct RetGen' lvalue prefix '*' cannot 
overflow
  `-CXXThisExpr 0x75a900 '__generic struct RetGen *' this

As I wasn't sure whether I should fix the initialization sequence again by 
splitting the address space conversion to move it to a later step or just fix 
the type of *this expr, I will upload a separate fix for this instead of 
reopening this review.

The overloading resolution seems to work fine at least which is what this test 
is supposed to check.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351747: [OpenCL] Allow address spaces as method qualifiers. 
(authored by stulova, committed by ).
Herald added a subscriber: ebevhan.

Changed prior to commit:
  https://reviews.llvm.org/D55850?vs=182496=182795#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55850

Files:
  include/clang/AST/Type.h
  include/clang/Sema/ParsedAttr.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
  test/SemaOpenCLCXX/address_space_overloading.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1982,7 +1982,7 @@
   bool isObjCQualifiedClassType() const;// Class
   bool isObjCObjectOrInterfaceType() const;
   bool isObjCIdType() const;// id
-
+  bool isDecltypeType() const;
   /// Was this type written with the special inert-in-ARC __unsafe_unretained
   /// qualifier?
   ///
@@ -6440,6 +6440,10 @@
   return isObjCIdType() || isObjCClassType() || isObjCSelType();
 }
 
+inline bool Type::isDecltypeType() const {
+  return isa(this);
+}
+
 #define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix) \
   inline bool Type::is##Id##Type() const { \
 return isSpecificBuiltinType(BuiltinType::Id); \
Index: include/clang/Sema/ParsedAttr.h
===
--- include/clang/Sema/ParsedAttr.h
+++ include/clang/Sema/ParsedAttr.h
@@ -567,6 +567,25 @@
   /// parsed attribute does not have a semantic equivalent, or would not have
   /// a Spelling enumeration, the value UINT_MAX is returned.
   unsigned getSemanticSpelling() const;
+
+  /// If this is an OpenCL addr space attribute returns its representation
+  /// in LangAS, otherwise returns default addr space.
+  LangAS asOpenCLLangAS() const {
+switch (getKind()) {
+case ParsedAttr::AT_OpenCLConstantAddressSpace:
+  return LangAS::opencl_constant;
+case ParsedAttr::AT_OpenCLGlobalAddressSpace:
+  return LangAS::opencl_global;
+case ParsedAttr::AT_OpenCLLocalAddressSpace:
+  return LangAS::opencl_local;
+case ParsedAttr::AT_OpenCLPrivateAddressSpace:
+  return LangAS::opencl_private;
+case ParsedAttr::AT_OpenCLGenericAddressSpace:
+  return LangAS::opencl_generic;
+default:
+  return LangAS::Default;
+}
+  }
 };
 
 class AttributePool;
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- test/CodeGenOpenCLCXX/method-overload-address-space.cl
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
===
--- test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
+++ test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
@@ -0,0 +1,18 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  auto fGlob() __global -> decltype(this);
+  auto fGen() -> decltype(this);
+  auto fErr() __global __local -> decltype(this); //expected-error{{multiple address spaces specified for type}}
+};
+
+void bar(__local C*);
+// expected-note@-1{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka '__global C *')), parameter type must be '__local C *'}}
+// expected-note@-2{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka 'C *')), parameter type must be '__local C *'}}
+
+__global C Glob;
+void foo(){

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

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

Okay, LGTM.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

Added a comment explaining when multiple address spaces are diagnosed.


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

https://reviews.llvm.org/D55850

Files:
  include/clang/AST/Type.h
  include/clang/Sema/ParsedAttr.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
@@ -0,0 +1,18 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  auto fGlob() __global -> decltype(this);
+  auto fGen() -> decltype(this);
+  auto fErr() __global __local -> decltype(this); //expected-error{{multiple address spaces specified for type}}
+};
+
+void bar(__local C*);
+// expected-note@-1{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka '__global C *')), parameter type must be '__local C *'}}
+// expected-note@-2{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka 'C *')), parameter type must be '__local C *'}}
+
+__global C Glob;
+void foo(){
+bar(Glob.fGlob()); // expected-error{{no matching function for call to 'bar'}}
+// FIXME: AS of 'this' below should be correctly deduced to generic
+bar(Glob.fGen()); // expected-error{{no matching function for call to 'bar'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3916,6 +3916,25 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+// Diagnose whether this is a case with the multiple addr spaces.
+// Returns true if this is an invalid case.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+static bool DiagnoseMultipleAddrSpaceAttributes(Sema , LangAS ASOld,
+LangAS ASNew,
+SourceLocation AttrLoc) {
+  if (ASOld != LangAS::Default) {
+if (ASOld != ASNew) {
+  S.Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+  return true;
+}
+// Emit a warning if they are identical; it's likely unintended.
+S.Diag(AttrLoc,
+   diag::warn_attribute_address_multiple_identical_qualifiers);
+  }
+  return false;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ 

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:6170
+  }
+}
+  }

Anastasia wrote:
> rjmccall wrote:
> > Does this not need to diagnose redundant qualifiers?  Why is this path 
> > required in addition to the path in SemaType, anyway?
> We discussed earlier to collect addr space qualifiers for completeness: 
> https://reviews.llvm.org/D55850#inline-495037
> 
> Without this change the following doesn't work for addr spaces correctly:
>   auto fGlob() __global -> decltype(this);
> I added a test that check this now in 
> **test/SemaOpenCLCXX/address-space-of-this-class-scope.cl**.
> 
> Here we only collect the quals to be applied to 'this'. If there are multiple 
> address space specified on a method it will be diagnosed in SemaType.cpp. I 
> think we probably don't need to give the same diagnostic twice?
> 
Okay, if this is already diagnosed, it isn't a problem.  Please leave a comment 
to that effect.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

- Fixed typo in FIXME


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

https://reviews.llvm.org/D55850

Files:
  include/clang/AST/Type.h
  include/clang/Sema/ParsedAttr.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
@@ -0,0 +1,18 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  auto fGlob() __global -> decltype(this);
+  auto fGen() -> decltype(this);
+  auto fErr() __global __local -> decltype(this); //expected-error{{multiple address spaces specified for type}}
+};
+
+void bar(__local C*);
+// expected-note@-1{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka '__global C *')), parameter type must be '__local C *'}}
+// expected-note@-2{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka 'C *')), parameter type must be '__local C *'}}
+
+__global C Glob;
+void foo(){
+bar(Glob.fGlob()); // expected-error{{no matching function for call to 'bar'}}
+// FIXME: AS of 'this' below should be correctly deduced to generic
+bar(Glob.fGen()); // expected-error{{no matching function for call to 'bar'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3916,6 +3916,25 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+// Diagnose whether this is a case with the multiple addr spaces.
+// Returns true if this is an invalid case.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+static bool DiagnoseMultipleAddrSpaceAttributes(Sema , LangAS ASOld,
+LangAS ASNew,
+SourceLocation AttrLoc) {
+  if (ASOld != LangAS::Default) {
+if (ASOld != ASNew) {
+  S.Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+  return true;
+}
+// Emit a warning if they are identical; it's likely unintended.
+S.Diag(AttrLoc,
+   diag::warn_attribute_address_multiple_identical_qualifiers);
+  }
+  return false;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ -4823,18 +4842,35 @@

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Parse/ParseDecl.cpp:6170
+  }
+}
+  }

rjmccall wrote:
> Does this not need to diagnose redundant qualifiers?  Why is this path 
> required in addition to the path in SemaType, anyway?
We discussed earlier to collect addr space qualifiers for completeness: 
https://reviews.llvm.org/D55850#inline-495037

Without this change the following doesn't work for addr spaces correctly:
  auto fGlob() __global -> decltype(this);
I added a test that check this now in 
**test/SemaOpenCLCXX/address-space-of-this-class-scope.cl**.

Here we only collect the quals to be applied to 'this'. If there are multiple 
address space specified on a method it will be diagnosed in SemaType.cpp. I 
think we probably don't need to give the same diagnostic twice?



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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

- Removed else case in DiagnoseMultipleAddrSpaceAttributes
- Added extra test to check correctness of addr space for 'this' when 
statically used in a class scope.


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

https://reviews.llvm.org/D55850

Files:
  include/clang/AST/Type.h
  include/clang/Sema/ParsedAttr.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/address-space-of-this-class-scope.cl
@@ -0,0 +1,18 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  auto fGlob() __global -> decltype(this);
+  auto fGen() -> decltype(this);
+  auto fErr() __global __local -> decltype(this); //expected-error{{multiple address spaces specified for type}}
+};
+
+void bar(__local C*);
+// expected-note@-1{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka '__global C *')), parameter type must be '__local C *'}}
+// expected-note@-2{{candidate function not viable: address space mismatch in 1st argument ('decltype(this)' (aka 'C *')), parameter type must be '__local C *'}}
+
+__global C Glob;
+void foo(){
+bar(Glob.fGlob()); // expected-error{{no matching function for call to 'bar'}}
+// FIXE: AS of 'this' below should be correctly deduced to generic
+bar(Glob.fGen()); // expected-error{{no matching function for call to 'bar'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3916,6 +3916,25 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+// Diagnose whether this is a case with the multiple addr spaces.
+// Returns true if this is an invalid case.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+static bool DiagnoseMultipleAddrSpaceAttributes(Sema , LangAS ASOld,
+LangAS ASNew,
+SourceLocation AttrLoc) {
+  if (ASOld != LangAS::Default) {
+if (ASOld != ASNew) {
+  S.Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+  return true;
+}
+// Emit a warning if they are identical; it's likely unintended.
+S.Diag(AttrLoc,
+   diag::warn_attribute_address_multiple_identical_qualifiers);
+  }
+  return false;
+}
+
 static TypeSourceInfo *
 

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:6170
+  }
+}
+  }

Does this not need to diagnose redundant qualifiers?  Why is this path required 
in addition to the path in SemaType, anyway?



Comment at: lib/Sema/SemaType.cpp:3930
+  return true;
+} else
+  // Emit a warning if they are identical; it's likely unintended.

I'd just drop this `else`, but if you want to include it, please use braces for 
consistency with the rest of the `if`-chain.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

Fixed wording on the comment.


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

https://reviews.llvm.org/D55850

Files:
  include/clang/Sema/ParsedAttr.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3916,6 +3916,25 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+// Diagnose whether this is a case with the multiple addr spaces.
+// Returns true if this is an invalid case.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+static bool DiagnoseMultipleAddrSpaceAttributes(Sema , LangAS ASOld,
+LangAS ASNew,
+SourceLocation AttrLoc) {
+  if (ASOld != LangAS::Default) {
+if (ASOld != ASNew) {
+  S.Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+  return true;
+} else
+  // Emit a warning if they are identical; it's likely unintended.
+  S.Diag(AttrLoc,
+ diag::warn_attribute_address_multiple_identical_qualifiers);
+  }
+  return false;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ -4823,18 +4842,35 @@
   Exceptions,
   EPI.ExceptionSpec);
 
-const auto  = D.getCXXScopeSpec();
+// FIXME: Set address space from attrs for C++ mode here.
 // OpenCLCPlusPlus: A class member function has an address space.
-if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
-((!Spec.isEmpty() &&
-  Spec.getScopeRep()->getKind() == NestedNameSpecifier::TypeSpec) ||
- state.getDeclarator().getContext() ==
- DeclaratorContext::MemberContext)) {
-  LangAS CurAS = EPI.TypeQuals.getAddressSpace();
+auto IsClassMember = [&]() {
+  return (!state.getDeclarator().getCXXScopeSpec().isEmpty() &&
+  state.getDeclarator()
+  .getCXXScopeSpec()
+  .getScopeRep()
+  ->getKind() == NestedNameSpecifier::TypeSpec) ||
+ state.getDeclarator().getContext() ==
+ DeclaratorContext::MemberContext;
+};
+
+if (state.getSema().getLangOpts().OpenCLCPlusPlus && IsClassMember()) {
+  LangAS ASIdx = LangAS::Default;
+  // Take address 

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

In D55850#1358812 , @rjmccall wrote:

> Okay, so is this ready to re-review independently?


Yes, please. It should be fine to review on its own. Thanks!


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Okay, so is this ready to re-review independently?


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:9279
+(CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default))
+  return true;
+  }

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > rjmccall wrote:
> > > > This at least needs a comment explaining the rule you're trying to 
> > > > implement.
> > > Okay, thanks for the clarification.  I think this should just be part of 
> > > `CompareImplicitConversionSequence`, right?  It seems to me that you 
> > > should prefer e.g. `int __private *` -> `int __private *` over `int 
> > > __generic *`  as a normal argument conversion as well.
> > > 
> > > Also, can this be written in terms of `isAddressSpaceSupersetOf`?  I 
> > > don't remember how `LangAS::Default` works in OpenCL C++ enough to 
> > > understand how it fits in here.
> > That's correct we should implement the same logic for the arguments too. I 
> > will create a helper function or do you think we should just call 
> > `CompareImplicitConversionSequence` on the method type too?
> > 
> > I think `isAddressSpaceSupersetOf` can't be used here because it determines 
> > compatibility of address spaces. However, the logic we are expressing is 
> > for the address space preference instead.
> If I understand correctly, we already call 
> `CompareImplicitConversionSequence` on the object-argument conversion above, 
> as part of the `for (unsigned ArgIdx = StartArg; ArgIdx < NumArgs; ++ArgIdx) 
> ` loop.
> 
> I think the address-space ordering rule can be reasonably based on 
> compatibility.  In fact, I think it already is in our implementation.  The 
> right rule is basically that (1) an exact match is better than a conversion 
> and (2) a conversion to a subspace is better than a conversion to a 
> superspace.  You can think of this as modifying the "proper subset" rule of 
> [over.ics.rank]p3.2.5, which in implementation terms means 
> `Qualifiers::isMoreQualifiedThan`, which already ends up using 
> `isAddressSpaceSupersetOf`.  So if this ranking isn't working right, it's 
> probably because we're incorrectly fast-pathing based on CVR qualifiers 
> somewhere, and in fact I can see a pretty suspicious check like that in 
> `CompareQualificationConversions`.
I created a separate review to fix this: https://reviews.llvm.org/D56735



Comment at: lib/Sema/SemaType.cpp:4877
   T = Context.getFunctionType(T, ParamTys, EPI);
   T = state.getSema().Context.getAddrSpaceQualType(T, AS);
 } else {

brunodf wrote:
> I follow all of the above (from the point "a class member function has an 
> address space"), but then I take issue with this line (already from Mikael).
> 
> You look at the declaration's attributes, to derive the ASIdx relating to the 
> method's this argument. You mark the relevant attributes as invalid, to 
> prevent them from being considered in "processTypeAttrs" after the switch 
> that we break below. The ASIdx is stored in the qualifiers EPI to go to the 
> FunctionProtoType (this will affect getThisType). This all seems fine.
> 
> But then this line adds the address space qualification to T, the type of the 
> declared function itself. This seems unnecessary, and conceptually wrong: 
> while the function may have an address space in syntax, this address space 
> applies to the this argument, not to the function object (and a pointer to 
> the function is not a pointer to this address space etc.). In short, I think 
> this line should be removed.
I think I fixed it in: https://reviews.llvm.org/D56066


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

Rebased on top of recent related changes and addressed remaining review 
comments.

This now depends on: https://reviews.llvm.org/D56735


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

https://reviews.llvm.org/D55850

Files:
  include/clang/Sema/ParsedAttr.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3916,6 +3916,25 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+// Diagnose whether this is a case with the multiple addr spaces.
+// Returns true if this is an invalid case.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+static bool DiagnoseMultipleAddrSpaceAttributes(Sema , LangAS ASOld,
+LangAS ASNew,
+SourceLocation AttrLoc) {
+  if (ASOld != LangAS::Default) {
+if (ASOld != ASNew) {
+  S.Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+  return true;
+} else
+  // Emit a warning if they are identical; it's likely unintended.
+  S.Diag(AttrLoc,
+ diag::warn_attribute_address_multiple_identical_qualifiers);
+  }
+  return false;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ -4823,18 +4842,35 @@
   Exceptions,
   EPI.ExceptionSpec);
 
-const auto  = D.getCXXScopeSpec();
+// FIXME: Set address space from attrs for C++ mode here.
 // OpenCLCPlusPlus: A class member function has an address space.
-if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
-((!Spec.isEmpty() &&
-  Spec.getScopeRep()->getKind() == NestedNameSpecifier::TypeSpec) ||
- state.getDeclarator().getContext() ==
- DeclaratorContext::MemberContext)) {
-  LangAS CurAS = EPI.TypeQuals.getAddressSpace();
+auto IsClassMember = [&]() {
+  return (!state.getDeclarator().getCXXScopeSpec().isEmpty() &&
+  state.getDeclarator()
+  .getCXXScopeSpec()
+  .getScopeRep()
+  ->getKind() == NestedNameSpecifier::TypeSpec) ||
+ state.getDeclarator().getContext() ==
+ DeclaratorContext::MemberContext;
+};
+
+if 

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added inline comments.



Comment at: lib/Sema/SemaType.cpp:4877
   T = Context.getFunctionType(T, ParamTys, EPI);
   T = state.getSema().Context.getAddrSpaceQualType(T, AS);
 } else {

I follow all of the above (from the point "a class member function has an 
address space"), but then I take issue with this line (already from Mikael).

You look at the declaration's attributes, to derive the ASIdx relating to the 
method's this argument. You mark the relevant attributes as invalid, to prevent 
them from being considered in "processTypeAttrs" after the switch that we break 
below. The ASIdx is stored in the qualifiers EPI to go to the FunctionProtoType 
(this will affect getThisType). This all seems fine.

But then this line adds the address space qualification to T, the type of the 
declared function itself. This seems unnecessary, and conceptually wrong: while 
the function may have an address space in syntax, this address space applies to 
the this argument, not to the function object (and a pointer to the function is 
not a pointer to this address space etc.). In short, I think this line should 
be removed.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:2828
 
   // FIXME: OpenCL: Need to consider address spaces
   unsigned FromQuals = FromFunction->getTypeQuals().getCVRUQualifiers();

rjmccall wrote:
> Anastasia wrote:
> > I am still missing something here.
> Well, at least the failure here is just to fall into the generic diagnostic.
> 
> Getting this diagnostic right probably requires some minor work to the 
> diagnostics engine.  If you look at `err_init_conversion_failed`, which is (I 
> think) the diagnostic that's always being used here, it matches every 
> possible CVR mask so that it can pretty-print them.  This is already a 
> problem because the input is actually a CVRU mask!  A better option would be 
> to teach `DiagnosticEngine` how to store and format a `Qualifiers` value, and 
> then you can just stream the original `Qualifiers` into the diagnostic here.
> 
> But that's obviously work for a separate patch.
I created a patch for this in https://reviews.llvm.org/D56198.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:9279
+(CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default))
+  return true;
+  }

Anastasia wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > This at least needs a comment explaining the rule you're trying to 
> > > implement.
> > Okay, thanks for the clarification.  I think this should just be part of 
> > `CompareImplicitConversionSequence`, right?  It seems to me that you should 
> > prefer e.g. `int __private *` -> `int __private *` over `int __generic *`  
> > as a normal argument conversion as well.
> > 
> > Also, can this be written in terms of `isAddressSpaceSupersetOf`?  I don't 
> > remember how `LangAS::Default` works in OpenCL C++ enough to understand how 
> > it fits in here.
> That's correct we should implement the same logic for the arguments too. I 
> will create a helper function or do you think we should just call 
> `CompareImplicitConversionSequence` on the method type too?
> 
> I think `isAddressSpaceSupersetOf` can't be used here because it determines 
> compatibility of address spaces. However, the logic we are expressing is for 
> the address space preference instead.
If I understand correctly, we already call `CompareImplicitConversionSequence` 
on the object-argument conversion above, as part of the `for (unsigned ArgIdx = 
StartArg; ArgIdx < NumArgs; ++ArgIdx) ` loop.

I think the address-space ordering rule can be reasonably based on 
compatibility.  In fact, I think it already is in our implementation.  The 
right rule is basically that (1) an exact match is better than a conversion and 
(2) a conversion to a subspace is better than a conversion to a superspace.  
You can think of this as modifying the "proper subset" rule of 
[over.ics.rank]p3.2.5, which in implementation terms means 
`Qualifiers::isMoreQualifiedThan`, which already ends up using 
`isAddressSpaceSupersetOf`.  So if this ranking isn't working right, it's 
probably because we're incorrectly fast-pathing based on CVR qualifiers 
somewhere, and in fact I can see a pretty suspicious check like that in 
`CompareQualificationConversions`.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:9279
+(CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default))
+  return true;
+  }

rjmccall wrote:
> rjmccall wrote:
> > This at least needs a comment explaining the rule you're trying to 
> > implement.
> Okay, thanks for the clarification.  I think this should just be part of 
> `CompareImplicitConversionSequence`, right?  It seems to me that you should 
> prefer e.g. `int __private *` -> `int __private *` over `int __generic *`  as 
> a normal argument conversion as well.
> 
> Also, can this be written in terms of `isAddressSpaceSupersetOf`?  I don't 
> remember how `LangAS::Default` works in OpenCL C++ enough to understand how 
> it fits in here.
That's correct we should implement the same logic for the arguments too. I will 
create a helper function or do you think we should just call 
`CompareImplicitConversionSequence` on the method type too?

I think `isAddressSpaceSupersetOf` can't be used here because it determines 
compatibility of address spaces. However, the logic we are expressing is for 
the address space preference instead.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:6162
+}
+  }
+

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > This is enforcing a restriction that users write `const __private`, 
> > > > which seems unreasonable.  It looks like `ParseTypeQualifierList` takes 
> > > > a flag saying not to parse attributes; try adding a new option to that 
> > > > enum allowing address-space attributes.
> > > > 
> > > > Collecting the attributes on the type-qualifiers `DeclSpec` rather than 
> > > > adding them as function attributes seems correct.
> > > Do you mean `ParseTypeQualifierListOpt`? That already parses the address 
> > > spaces unconditionally. The problem is however that we are using local 
> > > `DeclSpec` - `DS` variable here during the function qualifiers parsing 
> > > because the `DeclSpec` member of the `Declarator` corresponds to the 
> > > return type as far as I understand it. Therefore I am propagating missing 
> > > address space attributes from the local `DS` variable into `FnAttrs` to 
> > > be used in the function type info.
> > > 
> > > With current patch both of the following two forms:
> > >   struct C {
> > > void foo() __local const;
> > >   };
> > > and 
> > >   struct C {
> > > void foo() const __local;
> > >   };
> > > would be parsed correctly generating identical IR
> > >   declare void @_ZNU3AS3K1C3fooEv(%struct.C addrspace(3)*)
> > > 
> > > 
> > > 
> > Oh, I see, sorry.  Why filter on attributes at all, then?  We should *only* 
> > be parsing qualifier attributes in that list.
> > 
> > I actually think it would be reasonable to change 
> > `DeclaratorChunk::FunctionTypeInfo` to just store a `DeclSpec` for all the 
> > qualifiers; we're already duplicating an unfortunate amount of the logic 
> > from `DeclSpec`, like remembering `SourceLocation`s for all the qualifiers. 
> >  You'll have to store it out-of-line, but we already store e.g. parameters 
> > out of line, so the machinery for allocating and destroying it already 
> > exists.  Just make sure we don't actually allocate anything in the common 
> > case where there aren't any qualifiers (i.e. for C and non-member C++ 
> > functions).
> > 
> > Also, I suspect that the use of `CXXThisScopeRAII` below this needs to be 
> > updated to pull *all* the qualifiers out of `DS`.  Maybe Sema should have a 
> > function for that.
> I have uploaded a separate patch for this:
> https://reviews.llvm.org/D55948
> 
> I think I can't avoid storing `DeclSpec` for non-memeber functions in C++ 
> because we still use them to give diagnostics about the qualifiers. For 
> example:
>   test.cpp:1:12: error: non-member function cannot have 'const' qualifier
>   void foo() const;
> 
> As for `CXXThisScopeRAII`, we seem to be adding other qualifiers to the 
> `QualType` directly (while building it), so there seems to be no function to 
> collect them into `Qualifiers`. I can, however, add code to collect address 
> space qualifiers here. I guess we don't have any other missing qualifiers to 
> collect?
> 
I think you can probably avoid allocating and storing a `DeclSpec` if there are 
no qualifiers, which should be easy to test, and then the absence of a 
`DeclSpec` should allow Sema to quickly bail out of the check for illegal 
qualifiers on a non-member function type.

> I guess we don't have any other missing qualifiers to collect?

Not that are legal to apply to `this`.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Parse/ParseDecl.cpp:6162
+}
+  }
+

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > This is enforcing a restriction that users write `const __private`, which 
> > > seems unreasonable.  It looks like `ParseTypeQualifierList` takes a flag 
> > > saying not to parse attributes; try adding a new option to that enum 
> > > allowing address-space attributes.
> > > 
> > > Collecting the attributes on the type-qualifiers `DeclSpec` rather than 
> > > adding them as function attributes seems correct.
> > Do you mean `ParseTypeQualifierListOpt`? That already parses the address 
> > spaces unconditionally. The problem is however that we are using local 
> > `DeclSpec` - `DS` variable here during the function qualifiers parsing 
> > because the `DeclSpec` member of the `Declarator` corresponds to the return 
> > type as far as I understand it. Therefore I am propagating missing address 
> > space attributes from the local `DS` variable into `FnAttrs` to be used in 
> > the function type info.
> > 
> > With current patch both of the following two forms:
> >   struct C {
> > void foo() __local const;
> >   };
> > and 
> >   struct C {
> > void foo() const __local;
> >   };
> > would be parsed correctly generating identical IR
> >   declare void @_ZNU3AS3K1C3fooEv(%struct.C addrspace(3)*)
> > 
> > 
> > 
> Oh, I see, sorry.  Why filter on attributes at all, then?  We should *only* 
> be parsing qualifier attributes in that list.
> 
> I actually think it would be reasonable to change 
> `DeclaratorChunk::FunctionTypeInfo` to just store a `DeclSpec` for all the 
> qualifiers; we're already duplicating an unfortunate amount of the logic from 
> `DeclSpec`, like remembering `SourceLocation`s for all the qualifiers.  
> You'll have to store it out-of-line, but we already store e.g. parameters out 
> of line, so the machinery for allocating and destroying it already exists.  
> Just make sure we don't actually allocate anything in the common case where 
> there aren't any qualifiers (i.e. for C and non-member C++ functions).
> 
> Also, I suspect that the use of `CXXThisScopeRAII` below this needs to be 
> updated to pull *all* the qualifiers out of `DS`.  Maybe Sema should have a 
> function for that.
I have uploaded a separate patch for this:
https://reviews.llvm.org/D55948

I think I can't avoid storing `DeclSpec` for non-memeber functions in C++ 
because we still use them to give diagnostics about the qualifiers. For example:
  test.cpp:1:12: error: non-member function cannot have 'const' qualifier
  void foo() const;

As for `CXXThisScopeRAII`, we seem to be adding other qualifiers to the 
`QualType` directly (while building it), so there seems to be no function to 
collect them into `Qualifiers`. I can, however, add code to collect address 
space qualifiers here. I guess we don't have any other missing qualifiers to 
collect?



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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:6162
+}
+  }
+

Anastasia wrote:
> rjmccall wrote:
> > This is enforcing a restriction that users write `const __private`, which 
> > seems unreasonable.  It looks like `ParseTypeQualifierList` takes a flag 
> > saying not to parse attributes; try adding a new option to that enum 
> > allowing address-space attributes.
> > 
> > Collecting the attributes on the type-qualifiers `DeclSpec` rather than 
> > adding them as function attributes seems correct.
> Do you mean `ParseTypeQualifierListOpt`? That already parses the address 
> spaces unconditionally. The problem is however that we are using local 
> `DeclSpec` - `DS` variable here during the function qualifiers parsing 
> because the `DeclSpec` member of the `Declarator` corresponds to the return 
> type as far as I understand it. Therefore I am propagating missing address 
> space attributes from the local `DS` variable into `FnAttrs` to be used in 
> the function type info.
> 
> With current patch both of the following two forms:
>   struct C {
> void foo() __local const;
>   };
> and 
>   struct C {
> void foo() const __local;
>   };
> would be parsed correctly generating identical IR
>   declare void @_ZNU3AS3K1C3fooEv(%struct.C addrspace(3)*)
> 
> 
> 
Oh, I see, sorry.  Why filter on attributes at all, then?  We should *only* be 
parsing qualifier attributes in that list.

I actually think it would be reasonable to change 
`DeclaratorChunk::FunctionTypeInfo` to just store a `DeclSpec` for all the 
qualifiers; we're already duplicating an unfortunate amount of the logic from 
`DeclSpec`, like remembering `SourceLocation`s for all the qualifiers.  You'll 
have to store it out-of-line, but we already store e.g. parameters out of line, 
so the machinery for allocating and destroying it already exists.  Just make 
sure we don't actually allocate anything in the common case where there aren't 
any qualifiers (i.e. for C and non-member C++ functions).

Also, I suspect that the use of `CXXThisScopeRAII` below this needs to be 
updated to pull *all* the qualifiers out of `DS`.  Maybe Sema should have a 
function for that.



Comment at: lib/Sema/SemaOverload.cpp:2828
 
   // FIXME: OpenCL: Need to consider address spaces
   unsigned FromQuals = FromFunction->getTypeQuals().getCVRUQualifiers();

Anastasia wrote:
> I am still missing something here.
Well, at least the failure here is just to fall into the generic diagnostic.

Getting this diagnostic right probably requires some minor work to the 
diagnostics engine.  If you look at `err_init_conversion_failed`, which is (I 
think) the diagnostic that's always being used here, it matches every possible 
CVR mask so that it can pretty-print them.  This is already a problem because 
the input is actually a CVRU mask!  A better option would be to teach 
`DiagnosticEngine` how to store and format a `Qualifiers` value, and then you 
can just stream the original `Qualifiers` into the diagnostic here.

But that's obviously work for a separate patch.



Comment at: lib/Sema/SemaOverload.cpp:9279
+(CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default))
+  return true;
+  }

rjmccall wrote:
> This at least needs a comment explaining the rule you're trying to implement.
Okay, thanks for the clarification.  I think this should just be part of 
`CompareImplicitConversionSequence`, right?  It seems to me that you should 
prefer e.g. `int __private *` -> `int __private *` over `int __generic *`  as a 
normal argument conversion as well.

Also, can this be written in terms of `isAddressSpaceSupersetOf`?  I don't 
remember how `LangAS::Default` works in OpenCL C++ enough to understand how it 
fits in here.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D55850#1335826 , @rjmccall wrote:

> You're gating on OpenCL C++ in several places in this patch, but I don't 
> think you need to.  This extension should be available to anyone using 
> address spaces and C++.


Ok, I am happy to generalize it to C++. There are some slight differences in 
the address space attribute representation for C/C++ as I am now figuring out. 
So I would prefer to put a separate patch if it's ok with you.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:6162
+}
+  }
+

rjmccall wrote:
> This is enforcing a restriction that users write `const __private`, which 
> seems unreasonable.  It looks like `ParseTypeQualifierList` takes a flag 
> saying not to parse attributes; try adding a new option to that enum allowing 
> address-space attributes.
> 
> Collecting the attributes on the type-qualifiers `DeclSpec` rather than 
> adding them as function attributes seems correct.
Do you mean `ParseTypeQualifierListOpt`? That already parses the address spaces 
unconditionally. The problem is however that we are using local `DeclSpec` - 
`DS` variable here during the function qualifiers parsing because the 
`DeclSpec` member of the `Declarator` corresponds to the return type as far as 
I understand it. Therefore I am propagating missing address space attributes 
from the local `DS` variable into `FnAttrs` to be used in the function type 
info.

With current patch both of the following two forms:
  struct C {
void foo() __local const;
  };
and 
  struct C {
void foo() const __local;
  };
would be parsed correctly generating identical IR
  declare void @_ZNU3AS3K1C3fooEv(%struct.C addrspace(3)*)





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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

Address review comments.


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

https://reviews.llvm.org/D55850

Files:
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3921,6 +3921,14 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+/// IsClassMember - Determines whether a state belongs to a class member.
+static bool IsClassMember(TypeProcessingState ) {
+  return (!State.getDeclarator().getCXXScopeSpec().isEmpty() &&
+  State.getDeclarator().getCXXScopeSpec().getScopeRep()->getKind() ==
+  NestedNameSpecifier::TypeSpec) ||
+ State.getDeclarator().getContext() == DeclaratorContext::MemberContext;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ -4825,18 +4833,45 @@
   Exceptions,
   EPI.ExceptionSpec);
 
-const auto  = D.getCXXScopeSpec();
+// FIXME: Set address space from attrs for C++ mode here.
 // OpenCLCPlusPlus: A class member function has an address space.
 if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
-((!Spec.isEmpty() &&
-  Spec.getScopeRep()->getKind() == NestedNameSpecifier::TypeSpec) ||
- state.getDeclarator().getContext() ==
- DeclaratorContext::MemberContext)) {
-  LangAS CurAS = EPI.TypeQuals.getAddressSpace();
+IsClassMember(state)) {
+  LangAS ASIdx = LangAS::Default;
+  // Take address space attr if any and mark as invalid to avoid adding
+  // them later while creating QualType.
+  for (ParsedAttr  : DeclType.getAttrs()) {
+switch (attr.getKind()) {
+case ParsedAttr::AT_OpenCLConstantAddressSpace:
+  ASIdx = LangAS::opencl_constant;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLLocalAddressSpace:
+  ASIdx = LangAS::opencl_local;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLGlobalAddressSpace:
+  ASIdx = LangAS::opencl_global;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLPrivateAddressSpace:
+  ASIdx = LangAS::opencl_private;
+  attr.setInvalid();
+  break;
+case 

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

You're gating on OpenCL C++ in several places in this patch, but I don't think 
you need to.  This extension should be available to anyone using address spaces 
and C++.




Comment at: lib/Parse/ParseDecl.cpp:6162
+}
+  }
+

This is enforcing a restriction that users write `const __private`, which seems 
unreasonable.  It looks like `ParseTypeQualifierList` takes a flag saying not 
to parse attributes; try adding a new option to that enum allowing 
address-space attributes.

Collecting the attributes on the type-qualifiers `DeclSpec` rather than adding 
them as function attributes seems correct.



Comment at: lib/Sema/SemaOverload.cpp:1157
+NewMethod->getTypeQualifiers().getAddressSpace())
+  return true;
   }

If you leave `OldQuals` and `NewQuals` as `Qualifiers` and then just remove 
`restrict` from both sets, I think you don't need a special check for address 
spaces here.



Comment at: lib/Sema/SemaOverload.cpp:6671
+}
+  }
 }

I would expect this to fail in `TryObjectArgumentInitialization` above and not 
to need a special case here.



Comment at: lib/Sema/SemaOverload.cpp:9279
+(CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default))
+  return true;
+  }

This at least needs a comment explaining the rule you're trying to implement.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:2828
 
   // FIXME: OpenCL: Need to consider address spaces
   unsigned FromQuals = FromFunction->getTypeQuals().getCVRUQualifiers();

I am still missing something here.


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

https://reviews.llvm.org/D55850



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

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

Removed FIXME that has been addressed


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

https://reviews.llvm.org/D55850

Files:
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3921,6 +3921,14 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+/// IsClassMember - Determines whether a state belongs to a class member.
+static bool IsClassMember(TypeProcessingState ) {
+  return (!State.getDeclarator().getCXXScopeSpec().isEmpty() &&
+  State.getDeclarator().getCXXScopeSpec().getScopeRep()->getKind() ==
+  NestedNameSpecifier::TypeSpec) ||
+ State.getDeclarator().getContext() == DeclaratorContext::MemberContext;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ -4825,18 +4833,45 @@
   Exceptions,
   EPI.ExceptionSpec);
 
-const auto  = D.getCXXScopeSpec();
+// FIXME: Set address space from attrs for C++ mode here.
 // OpenCLCPlusPlus: A class member function has an address space.
 if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
-((!Spec.isEmpty() &&
-  Spec.getScopeRep()->getKind() == NestedNameSpecifier::TypeSpec) ||
- state.getDeclarator().getContext() ==
- DeclaratorContext::MemberContext)) {
-  LangAS CurAS = EPI.TypeQuals.getAddressSpace();
+IsClassMember(state)) {
+  LangAS ASIdx = LangAS::Default;
+  // Take address space attr if any and mark as invalid to avoid adding
+  // them later while creating QualType.
+  for (ParsedAttr  : DeclType.getAttrs()) {
+switch (attr.getKind()) {
+case ParsedAttr::AT_OpenCLConstantAddressSpace:
+  ASIdx = LangAS::opencl_constant;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLLocalAddressSpace:
+  ASIdx = LangAS::opencl_local;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLGlobalAddressSpace:
+  ASIdx = LangAS::opencl_global;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLPrivateAddressSpace:
+  ASIdx = LangAS::opencl_private;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLGenericAddressSpace:
+  ASIdx = 

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: rjmccall.
Herald added a subscriber: yaxunl.

This patch allows qualifying methods with address spaces and extends some 
overloading rules to use those qualifiers.

The main use case is to prevent conversions to generic/default address space 
where it's requested by the programmer. More details can be found in: 
http://lists.llvm.org/pipermail/cfe-dev/2018-December/060470.html

This patch doesn't enable the feature for C++ yet but it can easily be 
generalized if the overall flow is right.


https://reviews.llvm.org/D55850

Files:
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/method-overload-address-space.cl
  test/SemaOpenCLCXX/method-overload-address-space.cl

Index: test/SemaOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,20 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -pedantic -verify
+
+struct C {
+  void m1() __local __local; //expected-warning{{multiple identical address spaces specified for type}}
+  //expected-note@-1{{candidate function}}
+  void m1() __global;
+  //expected-note@-1{{candidate function}}
+  void m2() __global __local; //expected-error{{multiple address spaces specified for type}}
+};
+
+__global C c_glob;
+
+__kernel void bar() {
+  __local C c_loc;
+  C c_priv;
+
+  c_glob.m1();
+  c_loc.m1();
+  c_priv.m1(); //expected-error{{no matching member function for call to 'm1'}}
+}
Index: test/CodeGenOpenCLCXX/method-overload-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/method-overload-address-space.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct C {
+  void foo() __local;
+  void foo() __global;
+  void foo();
+  void bar();
+};
+
+__global C c1;
+
+__kernel void k() {
+  __local C c2;
+  C c3;
+  __global C _ref = c1;
+  __global C *c_ptr;
+
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c1.foo();
+  // CHECK: call void @_ZNU3AS31C3fooEv(%struct.C addrspace(3)*
+  c2.foo();
+  // CHECK: call void @_ZNU3AS41C3fooEv(%struct.C addrspace(4)*
+  c3.foo();
+  // CHECK: call void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ptr->foo();
+  // CHECK: void @_ZNU3AS11C3fooEv(%struct.C addrspace(1)*
+  c_ref.foo();
+
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c1.bar();
+  //FIXME: Doesn't compile yet
+  //c_ptr->bar();
+  // CHECK: call void @_ZNU3AS41C3barEv(%struct.C addrspace(4)* addrspacecast (%struct.C addrspace(1)* @c1 to %struct.C addrspace(4)*))
+  c_ref.bar();
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -3921,6 +3921,14 @@
   llvm_unreachable("unknown NullabilityKind");
 }
 
+/// IsClassMember - Determines whether a state belongs to a class member.
+static bool IsClassMember(TypeProcessingState ) {
+  return (!State.getDeclarator().getCXXScopeSpec().isEmpty() &&
+  State.getDeclarator().getCXXScopeSpec().getScopeRep()->getKind() ==
+  NestedNameSpecifier::TypeSpec) ||
+ State.getDeclarator().getContext() == DeclaratorContext::MemberContext;
+}
+
 static TypeSourceInfo *
 GetTypeSourceInfoForDeclarator(TypeProcessingState ,
QualType T, TypeSourceInfo *ReturnTypeInfo);
@@ -4825,18 +4833,45 @@
   Exceptions,
   EPI.ExceptionSpec);
 
-const auto  = D.getCXXScopeSpec();
+// FIXME: Set address space from attrs for C++ mode here.
 // OpenCLCPlusPlus: A class member function has an address space.
 if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
-((!Spec.isEmpty() &&
-  Spec.getScopeRep()->getKind() == NestedNameSpecifier::TypeSpec) ||
- state.getDeclarator().getContext() ==
- DeclaratorContext::MemberContext)) {
-  LangAS CurAS = EPI.TypeQuals.getAddressSpace();
+IsClassMember(state)) {
+  LangAS ASIdx = LangAS::Default;
+  // Take address space attr if any and mark as invalid to avoid adding
+  // them later while creating QualType.
+  for (ParsedAttr  : DeclType.getAttrs()) {
+switch (attr.getKind()) {
+case ParsedAttr::AT_OpenCLConstantAddressSpace:
+  ASIdx = LangAS::opencl_constant;
+  attr.setInvalid();
+  break;
+case ParsedAttr::AT_OpenCLLocalAddressSpace:
+  ASIdx = LangAS::opencl_local;
+  attr.setInvalid();
+  break;
+case