[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D54862#1333700 , @ebevhan wrote:

> I'm also a bit confused about the semantics that this patch is applying to 
> function types. It mostly seems to concern the extra trailing Qualifiers on 
> CXXMethodDecl to store the addrspace quals, but in some places 
> (SemaType:4842, SemaDecl:3198) it seems to be applying the address space to 
> the function type itself, which certainly seems like something else to me. A 
> function with an address space qualified type would (to me, at least) be a 
> function *located* in that address space, not one qualified to take a `this` 
> from that address space.


Yeah, there may be some confusion about that, and unfortunately it's easy to 
miss in the review.

But yes, I agree that address-space qualifiers are in principle one of the few 
qualifiers that make sense to be able to have (in a normal way) on function 
types.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I'm also a bit confused about the semantics that this patch is applying to 
function types. It mostly seems to concern the extra trailing Qualifiers on 
CXXMethodDecl to store the addrspace quals, but in some places (SemaType:4842, 
SemaDecl:3198) it seems to be applying the address space to the function type 
itself, which certainly seems like something else to me. A function with an 
address space qualified type would (to me, at least) be a function *located* in 
that address space, not one qualified to take a `this` from that address space.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:77
+  if (MD)
+RecTy = Context.getAddrSpaceQualType(RecTy, 
MD->getType().getAddressSpace());
   return Context.getPointerType(CanQualType::CreateUnsafe(RecTy));

ebevhan wrote:
> I'm a bit late to the party, but shouldn't this be 
> `MD->getTypeQualifiers().getAddressSpace()`?
Yes, you're right, it should.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:77
+  if (MD)
+RecTy = Context.getAddrSpaceQualType(RecTy, 
MD->getType().getAddressSpace());
   return Context.getPointerType(CanQualType::CreateUnsafe(RecTy));

I'm a bit late to the party, but shouldn't this be 
`MD->getTypeQualifiers().getAddressSpace()`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D54862#1328290 , @mikael wrote:

> Seems like my this commit broke: 
> http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/33176
>
> But since I don't really know what anything about lldb, I probably won't be 
> able to fix it fast enough.
>
>   
> /lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/lldb/source/Symbol/ClangASTContext.cpp:2208:24:
>  error: no viable overloaded '='
> proto_info.TypeQuals = type_quals;
>  ^ ~~
>   
> /lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7:
>  note: candidate function (the implicit copy assignment operator) not viable: 
> no known conversion from 'unsigned int' to 'const clang::Qualifiers' for 1st 
> argument
>   class Qualifiers {
> ^
>   
> /lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7:
>  note: candidate function (the implicit move assignment operator) not viable: 
> no known conversion from 'unsigned int' to 'clang::Qualifiers' for 1st 
> argument
>   1 error generated.
>
>
> I think it can be easily solved by Qualifiers::fromFastMask(type_quals); also 
> updating the variable name since it changed.


I think that's exactly what you need to do, and you'd be welcome to commit that 
instead of reverting this.




Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

akyrtzi wrote:
> rjmccall wrote:
> > akyrtzi wrote:
> > > rjmccall wrote:
> > > > Paging @akyrtzi here.  The address-space qualifier should be part of 
> > > > the USR but I don't think if there's a defined schema for that.
> > > If I understand correctly from other comments you can't add special 
> > > mangling and USR generation yet and intend to add FIXME for doing 
> > > mangling support later ? If yes, could you also add FIXME here and 
> > > re-visit ?
> > While you're here, can you described the right way for us to extend USR 
> > generation here to potentially add an address space?
> I'm not familiar with the mechanism, is address space supposed to be 
> identified via a single integer ? If so, then I think adding '-' character 
> plus the integer as character for the address space, immediately after the 
> type qualifiers, should be a way to go.
Okay.  Some of them are integers; others are identified in the language as more 
of a keyword and so there's no stable mapping from an integer value.  Should we 
just come up with some simple schema for that following `-` that's unambiguous 
with a digit?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/AST/Type.h:3692
   FunctionType::ExceptionType, Expr *, FunctionDecl *,
-  FunctionType::ExtParameterInfo> {
   friend class ASTContext; // ASTContext creates these.

You can use `FunctionType::FunctionTypeExtraBitfields`
to store the qualifiers. It was designed to store extra data.
No need to add yet another trailing object.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael added a comment.

Seems like my this commit broke: 
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/33176

But since I don't really know what anything about lldb, I probably won't be 
able to fix it fast enough.

  
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/lldb/source/Symbol/ClangASTContext.cpp:2208:24:
 error: no viable overloaded '='
proto_info.TypeQuals = type_quals;
 ^ ~~
  
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7:
 note: candidate function (the implicit copy assignment operator) not viable: 
no known conversion from 'unsigned int' to 'const clang::Qualifiers' for 1st 
argument
  class Qualifiers {
^
  
/lldb-buildbot/buildServerSlave/lldb-android-buildserver/llvm/tools/clang/include/clang/AST/Type.h:141:7:
 note: candidate function (the implicit move assignment operator) not viable: 
no known conversion from 'unsigned int' to 'clang::Qualifiers' for 1st argument
  1 error generated.

I think it can be easily solved by Qualifiers::fromFastMask(type_quals); also 
updating the variable name since it changed.

But I know nothing about lldb, so I better revert this patch for now.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348927: [OpenCL] Add generic AS to this pointer 
(authored by mikael, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54862?vs=177477=177846#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  test/CodeGenOpenCLCXX/template-address-spaces.cl
  test/SemaOpenCLCXX/address-space-templates.cl
  tools/libclang/CIndex.cpp

Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -270,7 +270,8 @@
   if (const CXXMethodDecl *MD = dyn_cast(D)) {
 if (MD->isStatic())
   Out << 'S';
-if (unsigned quals = MD->getTypeQualifiers())
+// FIXME: OpenCL: Need to consider address spaces
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {
 case RQ_None: break;
Index: lib/CodeGen/CGClass.cpp
===
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -16,6 +16,7 @@
 #include "CGDebugInfo.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
+#include "TargetInfo.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
@@ -2012,8 +2013,19 @@
  bool NewPointerIsChecked) {
   CallArgList Args;
 
+  LangAS SlotAS = E->getType().getAddressSpace();
+  QualType ThisType = D->getThisType(getContext());
+  LangAS ThisAS = ThisType.getTypePtr()->getPointeeType().getAddressSpace();
+  llvm::Value *ThisPtr = This.getPointer();
+  if (SlotAS != ThisAS) {
+unsigned TargetThisAS = getContext().getTargetAddressSpace(ThisAS);
+llvm::Type *NewType =
+ThisPtr->getType()->getPointerElementType()->getPointerTo(TargetThisAS);
+ThisPtr = getTargetHooks().performAddrSpaceCast(*this, This.getPointer(),
+ThisAS, SlotAS, NewType);
+  }
   // Push the this ptr.
-  Args.add(RValue::get(This.getPointer()), D->getThisType(getContext()));
+  Args.add(RValue::get(ThisPtr), D->getThisType(getContext()));
 
   // If this is a trivial constructor, emit a memcpy now before we lose
   // the alignment information on the argument.
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -2617,9 +2617,9 @@
   const FunctionProtoType *FPT =
   Ty->getPointeeType()->getAs();
   return DBuilder.createMemberPointerType(
-  getOrCreateInstanceMethodType(CGM.getContext().getPointerType(QualType(
-Ty->getClass(), FPT->getTypeQuals())),
-FPT, U),
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),
   ClassType, Size, /*Align=*/0, Flags);
 }
 
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -26,7 +26,10 @@
 
 static void EmitDeclInit(CodeGenFunction , const VarDecl ,
  ConstantAddress DeclPtr) {
-  assert(D.hasGlobalStorage() && "VarDecl must have global storage!");
+  assert(
+  (D.hasGlobalStorage() ||
+   (D.hasLocalStorage() && CGF.getContext().getLangOpts().OpenCLCPlusPlus)) &&
+  "VarDecl must have global or local (in the case of OpenCL) storage!");
   assert(!D.getType()->isReferenceType() &&
  "Should not call EmitDeclInit on a reference!");
 
Index: lib/CodeGen/CGValue.h

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.

LGTM! Apart from small improvement that can done before committing if it works 
at all. :)




Comment at: lib/Sema/SemaType.cpp:5169
+TypeSourceInfo *Sema::GetTypeForDeclarator(Declarator , Scope *S,
+   const DeclContext *DC) {
   // Determine the type of the declarator. Not all forms of declarator

mikael wrote:
> Anastasia wrote:
> > Rather than passing `DeclContext` you can get it from the `Declarator` 
> > using `getContext()` method.
> Declarator::getContext() returns an enumeration DeclaratorContext which is 
> not enough in this case.
Ok, I was wondering if you can use scope 
`clang::NestedNameSpecifier::SpecifierKind` to check if something is a method 
instead of passing `DC` down.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaType.cpp:5169
+TypeSourceInfo *Sema::GetTypeForDeclarator(Declarator , Scope *S,
+   const DeclContext *DC) {
   // Determine the type of the declarator. Not all forms of declarator

Anastasia wrote:
> Rather than passing `DeclContext` you can get it from the `Declarator` using 
> `getContext()` method.
Declarator::getContext() returns an enumeration DeclaratorContext which is not 
enough in this case.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: test/SemaOpenCLCXX/address-space-templates.cl:7
   T f1(); // expected-error{{function type may not be qualified with an 
address space}}
-  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
+  // FIXME: Should only get the error message once.
+  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}} expected-error{{parameter may not be qualified with an address 
space}}

Anastasia wrote:
> mikael wrote:
> > This was the remaining issue that I have not solved yet. It looked like 
> > this issue was not so trivial so I think it makes sense to postpone it.
> Do you know why it appears twice?
I believe the main issue is that  we have some code that does the following in 
SemaTemplateInstantiateDecl.cpp:

```
OldTL.getAs()
```

But the implementation of getAs returns nullptr if you have local modifiers. So 
since this code will always return nullptr we end up taking a different paths.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaType.cpp:3929
+static TypeSourceInfo *
+GetFullTypeForDeclarator(TypeProcessingState , QualType declSpecType,
+ TypeSourceInfo *TInfo,

From the `state` you can use `getDeclarator()` that then call a method 
`getContext()`.



Comment at: lib/Sema/SemaType.cpp:5169
+TypeSourceInfo *Sema::GetTypeForDeclarator(Declarator , Scope *S,
+   const DeclContext *DC) {
   // Determine the type of the declarator. Not all forms of declarator

Rather than passing `DeclContext` you can get it from the `Declarator` using 
`getContext()` method.



Comment at: lib/Sema/TreeTransform.h:4277
   //   cv-qualifiers are ignored.
-  if (T->isFunctionType())
+  // OpenCL: The address space should not be ignored.
+  if (T->isFunctionType()) {

mikael wrote:
> Another place where the address space was removed.
Why OpenCL in the comment, since the code seems to be rather generic.

Also not sure this comments adds anything that can't be understood from the 
code you are adding.



Comment at: test/SemaOpenCLCXX/address-space-templates.cl:7
   T f1(); // expected-error{{function type may not be qualified with an 
address space}}
-  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
+  // FIXME: Should only get the error message once.
+  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}} expected-error{{parameter may not be qualified with an address 
space}}

mikael wrote:
> This was the remaining issue that I have not solved yet. It looked like this 
> issue was not so trivial so I think it makes sense to postpone it.
Do you know why it appears twice?


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael marked 4 inline comments as done.
mikael added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:3196
+
+QualType AdjustedQT = QualType(AdjustedType, 0);
+LangAS AS = Old->getType().getAddressSpace();

When merging the class function and the file context function the address space 
was lost here.



Comment at: lib/Sema/SemaType.cpp:4831
+if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
+((DC && DC->isRecord()) ||
+ state.getDeclarator().getContext() ==

The first issue was that if a function is declared outside of a class this code 
did not deduce it. Now it does.



Comment at: lib/Sema/TreeTransform.h:4277
   //   cv-qualifiers are ignored.
-  if (T->isFunctionType())
+  // OpenCL: The address space should not be ignored.
+  if (T->isFunctionType()) {

Another place where the address space was removed.



Comment at: test/SemaOpenCLCXX/address-space-templates.cl:7
   T f1(); // expected-error{{function type may not be qualified with an 
address space}}
-  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
+  // FIXME: Should only get the error message once.
+  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}} expected-error{{parameter may not be qualified with an address 
space}}

This was the remaining issue that I have not solved yet. It looked like this 
issue was not so trivial so I think it makes sense to postpone it.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael added a comment.

I rebased on Friday, and noticed that I broke two tests:

Failing Tests (2):

  Clang :: CodeGenOpenCLCXX/template-address-spaces.cl
  Clang :: SemaOpenCLCXX/address-space-templates.cl

This upload contains a few extra fixes.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 177477.
mikael marked an inline comment as not done.

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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  test/CodeGenOpenCLCXX/template-address-spaces.cl
  test/SemaOpenCLCXX/address-space-templates.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -4,7 +4,9 @@
 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}}
-  void f2(T); // expected-error{{parameter may not be qualified with an address space}}
+  // FIXME: Should only get the error message once.
+  void f2(T); // expected-error{{parameter may not be qualified with an address space}} expected-error{{parameter may not be qualified with an address space}}
+
 };
 
 template 
Index: test/CodeGenOpenCLCXX/template-address-spaces.cl
===
--- test/CodeGenOpenCLCXX/template-address-spaces.cl
+++ test/CodeGenOpenCLCXX/template-address-spaces.cl
@@ -9,13 +9,16 @@
 template
 T S::foo() { return a;}
 
-//CHECK: %struct.S = type { i32 }
-//CHECK: %struct.S.0 = type { i32 addrspace(4)* }
-//CHECK: %struct.S.1 = type { i32 addrspace(1)* }
+// CHECK: %struct.S = type { i32 }
+// CHECK: %struct.S.0 = type { i32 addrspace(4)* }
+// CHECK: %struct.S.1 = type { i32 addrspace(1)* }
 
-//CHECK: i32 @_ZN1SIiE3fooEv(%struct.S* %this)
-//CHECK: i32 addrspace(4)* @_ZN1SIPU3AS4iE3fooEv(%struct.S.0* %this)
-//CHECK: i32 addrspace(1)* @_ZN1SIPU3AS1iE3fooEv(%struct.S.1* %this)
+// CHECK:  %0 = addrspacecast %struct.S* %sint to %struct.S addrspace(4)*
+// CHECK:  %call = call i32 @_ZNU3AS41SIiE3fooEv(%struct.S addrspace(4)* %0) #1
+// CHECK:  %1 = addrspacecast %struct.S.0* %sintptr to %struct.S.0 addrspace(4)*
+// CHECK:  %call1 = call i32 addrspace(4)* @_ZNU3AS41SIPU3AS4iE3fooEv(%struct.S.0 addrspace(4)* %1) #1
+// CHECK:  %2 = addrspacecast %struct.S.1* %sintptrgl to %struct.S.1 addrspace(4)*
+// CHECK:  %call2 = call i32 addrspace(1)* @_ZNU3AS41SIPU3AS1iE3fooEv(%struct.S.1 addrspace(4)* %2) #1
 
 void bar(){
   S sint;
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,154 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  // FIXME: Does not work yet.
+  // C(C &) { v = c.v; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  // FIXME: Does not work yet.
+  //C =(C&& c) & {
+  //  v = c.v;
+  //  return *this;
+  //}
+
+  int get() { return v; }
+
+  int outside();
+};
+
+int C::outside() {
+  return v;
+}
+
+extern C&& foo();
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  int i2 = c.outside();
+  C c1(c);
+  C c2;
+  c2 = c1;
+  // FIXME: 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

rjmccall wrote:
> akyrtzi wrote:
> > rjmccall wrote:
> > > Paging @akyrtzi here.  The address-space qualifier should be part of the 
> > > USR but I don't think if there's a defined schema for that.
> > If I understand correctly from other comments you can't add special 
> > mangling and USR generation yet and intend to add FIXME for doing mangling 
> > support later ? If yes, could you also add FIXME here and re-visit ?
> While you're here, can you described the right way for us to extend USR 
> generation here to potentially add an address space?
I'm not familiar with the mechanism, is address space supposed to be identified 
via a single integer ? If so, then I think adding '-' character plus the 
integer as character for the address space, immediately after the type 
qualifiers, should be a way to go.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

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

LGTM, but you can probably undo one of my requests. :)




Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

mikael wrote:
> mikael wrote:
> > rjmccall wrote:
> > > The indentation here is messed up.
> > > 
> > > You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> > > places.  That's currently correct, in the sense that the fast qualifiers 
> > > are currently defined to be the CVR qualifiers, but the point of the 
> > > distinction is that we might want to change that (and we have in fact 
> > > explored that in the past).  I think you should make `FunctionTypeBits` 
> > > only claim to store the fast qualifiers, which mostly just means updating 
> > > a few field / accessor names and changing things like the 
> > > `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.
> > > 
> > > If there are places where it's convenient to assume that the CVR 
> > > qualifiers are exactly the fast qualifiers, it's okay to static_assert 
> > > that, but you should make sure to static_assert it in each place you 
> > > assume it.
> > I'll change it, but I do have a question related to this:
> > 
> > Can we make any assumptions with the "fast qualifiers"? I'd like it if we 
> > can assume that they fit in 3 bits. Otherwise I think I also need an 
> > assertion here.
> I solved it like this in the end:
> 
> * I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to 
> make it dependent on possible changes to the fast flags.
> * Then I put a static_assertion to guard the hastConst(), hasVolatile() and 
> hasRestrict() methods.
Great, looks good.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),

mikael wrote:
> rjmccall wrote:
> > Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
> Your initial comment suggested to send in a record rather than a type. But I 
> see now that it may make more sense to use the type directly instead. Will 
> update it.
Oh, I actually didn't even think about the fact that the `getClass())` returns 
a `Type*` instead of a `CXXRecordDecl`.  Using the record decl is probably 
better, sorry.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael marked an inline comment as done and an inline comment as not done.
mikael added inline comments.



Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

mikael wrote:
> rjmccall wrote:
> > The indentation here is messed up.
> > 
> > You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> > places.  That's currently correct, in the sense that the fast qualifiers 
> > are currently defined to be the CVR qualifiers, but the point of the 
> > distinction is that we might want to change that (and we have in fact 
> > explored that in the past).  I think you should make `FunctionTypeBits` 
> > only claim to store the fast qualifiers, which mostly just means updating a 
> > few field / accessor names and changing things like the 
> > `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.
> > 
> > If there are places where it's convenient to assume that the CVR qualifiers 
> > are exactly the fast qualifiers, it's okay to static_assert that, but you 
> > should make sure to static_assert it in each place you assume it.
> I'll change it, but I do have a question related to this:
> 
> Can we make any assumptions with the "fast qualifiers"? I'd like it if we can 
> assume that they fit in 3 bits. Otherwise I think I also need an assertion 
> here.
I solved it like this in the end:

* I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to make 
it dependent on possible changes to the fast flags.
* Then I put a static_assertion to guard the hastConst(), hasVolatile() and 
hasRestrict() methods.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 176943.
mikael added a comment.

- Added FIXME's.
- Fixed the latest comments.


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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZNU3AS41C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZNU3AS41CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %1) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZNU3AS41CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

rjmccall wrote:
> The indentation here is messed up.
> 
> You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> places.  That's currently correct, in the sense that the fast qualifiers are 
> currently defined to be the CVR qualifiers, but the point of the distinction 
> is that we might want to change that (and we have in fact explored that in 
> the past).  I think you should make `FunctionTypeBits` only claim to store 
> the fast qualifiers, which mostly just means updating a few field / accessor 
> names and changing things like the `getCVRQualifiers()` call right above this 
> to be `getFastQualifiers()`.
> 
> If there are places where it's convenient to assume that the CVR qualifiers 
> are exactly the fast qualifiers, it's okay to static_assert that, but you 
> should make sure to static_assert it in each place you assume it.
I'll change it, but I do have a question related to this:

Can we make any assumptions with the "fast qualifiers"? I'd like it if we can 
assume that they fit in 3 bits. Otherwise I think I also need an assertion here.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),

rjmccall wrote:
> Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
Your initial comment suggested to send in a record rather than a type. But I 
see now that it may make more sense to use the type directly instead. Will 
update it.



Comment at: lib/CodeGen/CGDeclCXX.cpp:32
+   (D.hasLocalStorage() && 
CGF.getContext().getLangOpts().OpenCLCPlusPlus)) &&
+  "VarDecl must have global or local (in the case of OpenCL) storage!");
   assert(!D.getType()->isReferenceType() &&

rjmccall wrote:
> What causes OpenCL C++ declarations to go down this path?
It goes there when we defined an object  in __local address space.  From the 
spec "The constructors of a objects in local memory should only be initialized 
once".  This is the same case as with C++ 11 static local variables.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/AST/ItaniumMangle.cpp:1507
+Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+Method->getTypeQualifiers().getCVRUQualifiers());
 // We do not consider restrict a distinguishing attribute for overloading

mikael wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > You can overload based on the address space, right?  I think it needs 
> > > > to be mangled.
> > > Does this refer to our earlier discussion 
> > > https://reviews.llvm.org/D54862#inline-484509
> > > 
> > > We don't have a way to qualify methods with an address space yet? I was 
> > > going to send an RFC to `cfe-dev` for this but if you think it would be 
> > > ok to go ahead with an implementation, I am happy with it. Either way 
> > > would it be better to do this in a separate patch?
> > I'm fine with delaying implementation on these two issues until a later 
> > patch since, as you say, they can't be tested well until we support 
> > arbitrary address-space qualifiers.  Please at least leave FIXMEs for them.
> I actually did update this, since we can test the mangling of __generic in 
> the test.
Great, thanks.  Looks good.



Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

The indentation here is messed up.

You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
places.  That's currently correct, in the sense that the fast qualifiers are 
currently defined to be the CVR qualifiers, but the point of the distinction is 
that we might want to change that (and we have in fact explored that in the 
past).  I think you should make `FunctionTypeBits` only claim to store the fast 
qualifiers, which mostly just means updating a few field / accessor names and 
changing things like the `getCVRQualifiers()` call right above this to be 
`getFastQualifiers()`.

If there are places where it's convenient to assume that the CVR qualifiers are 
exactly the fast qualifiers, it's okay to static_assert that, but you should 
make sure to static_assert it in each place you assume it.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),

Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?



Comment at: lib/CodeGen/CGDeclCXX.cpp:32
+   (D.hasLocalStorage() && 
CGF.getContext().getLangOpts().OpenCLCPlusPlus)) &&
+  "VarDecl must have global or local (in the case of OpenCL) storage!");
   assert(!D.getType()->isReferenceType() &&

What causes OpenCL C++ declarations to go down this path?



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

akyrtzi wrote:
> rjmccall wrote:
> > Paging @akyrtzi here.  The address-space qualifier should be part of the 
> > USR but I don't think if there's a defined schema for that.
> If I understand correctly from other comments you can't add special mangling 
> and USR generation yet and intend to add FIXME for doing mangling support 
> later ? If yes, could you also add FIXME here and re-visit ?
While you're here, can you described the right way for us to extend USR 
generation here to potentially add an address space?



Comment at: lib/Parse/ParseDecl.cpp:6166
+
+  Qualifiers Q = Qualifiers::fromOpaqueValue(DS.getTypeQualifiers());
+  if (D.getDeclSpec().isConstexprSpecified() && !getLangOpts().CPlusPlus14)

I think `fromCVRUQualifiers` is probably the right function, and if that 
doesn't exist you should add it.



Comment at: lib/Sema/SemaExprCXX.cpp:1112
+  QualType T = S.Context.getRecordType(Record).withCVRQualifiers(Quals);
+  T = S.getASTContext().getAddrSpaceQualType(T, 
CXXThisTypeQuals.getAddressSpace());
+

I don't think there's a good reason for this to be dropping non-CVR qualifiers. 
 If there is, it really needs a comment.



Comment at: lib/Sema/TreeTransform.h:5276
+  Sema::CXXThisScopeRAII ThisScope(
+  SemaRef, ThisContext, Qualifiers::fromOpaqueValue(ThisTypeQuals));
 

I think you need to turn `ThisTypeQuals` into a `Qualifiers` here.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

rjmccall wrote:
> Paging @akyrtzi here.  The address-space qualifier should be part of the USR 
> but I don't think if there's a defined schema for that.
If I understand correctly from other comments you can't add special mangling 
and USR generation yet and intend to add FIXME for doing mangling support later 
? If yes, could you also add FIXME here and re-visit ?


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/AST/ItaniumMangle.cpp:1507
+Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+Method->getTypeQualifiers().getCVRUQualifiers());
 // We do not consider restrict a distinguishing attribute for overloading

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > You can overload based on the address space, right?  I think it needs to 
> > > be mangled.
> > Does this refer to our earlier discussion 
> > https://reviews.llvm.org/D54862#inline-484509
> > 
> > We don't have a way to qualify methods with an address space yet? I was 
> > going to send an RFC to `cfe-dev` for this but if you think it would be ok 
> > to go ahead with an implementation, I am happy with it. Either way would it 
> > be better to do this in a separate patch?
> I'm fine with delaying implementation on these two issues until a later patch 
> since, as you say, they can't be tested well until we support arbitrary 
> address-space qualifiers.  Please at least leave FIXMEs for them.
I actually did update this, since we can test the mangling of __generic in the 
test.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 176831.
mikael added a comment.

I uploaded a new patch for most of the comments. I did leave some things out 
since they need clarification.


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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZNU3AS41C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZNU3AS41CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %1) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZNU3AS41CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:1146
+unsigned OldQuals = OldMethod->getTypeQualifiers().getCVRUQualifiers();
+unsigned NewQuals = NewMethod->getTypeQualifiers().getCVRUQualifiers();
 if (!getLangOpts().CPlusPlus14 && NewMethod->isConstexpr() &&

mikael wrote:
> rjmccall wrote:
> > This is an algorithm that I think you need to get right and which shouldn't 
> > drop address spaces.
> But with this patch we can only have __generic address space for methods. So 
> if I try to implement something here I can't really test it? Or am I missing 
> something?
Since we didn't add address spaces to the method qualifiers yet, it seems 
logical to ignore them.

Although we could already say, since we would like to add such overloading 
anyway, it would be logical to return true for the address space qualifiers 
mismatch just like it's done for CV quals in line 1154. However, I can't think 
of any way we could test this indeed.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/AST/ItaniumMangle.cpp:1507
+Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+Method->getTypeQualifiers().getCVRUQualifiers());
 // We do not consider restrict a distinguishing attribute for overloading

Anastasia wrote:
> rjmccall wrote:
> > You can overload based on the address space, right?  I think it needs to be 
> > mangled.
> Does this refer to our earlier discussion 
> https://reviews.llvm.org/D54862#inline-484509
> 
> We don't have a way to qualify methods with an address space yet? I was going 
> to send an RFC to `cfe-dev` for this but if you think it would be ok to go 
> ahead with an implementation, I am happy with it. Either way would it be 
> better to do this in a separate patch?
I'm fine with delaying implementation on these two issues until a later patch 
since, as you say, they can't be tested well until we support arbitrary 
address-space qualifiers.  Please at least leave FIXMEs for them.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/AST/ItaniumMangle.cpp:1507
+Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+Method->getTypeQualifiers().getCVRUQualifiers());
 // We do not consider restrict a distinguishing attribute for overloading

rjmccall wrote:
> You can overload based on the address space, right?  I think it needs to be 
> mangled.
Does this refer to our earlier discussion 
https://reviews.llvm.org/D54862#inline-484509

We don't have a way to qualify methods with an address space yet? I was going 
to send an RFC to `cfe-dev` for this but if you think it would be ok to go 
ahead with an implementation, I am happy with it. Either way would it be better 
to do this in a separate patch?


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael marked an inline comment as done and an inline comment as not done.
mikael added a comment.

Thanks for the feedback, I'll work on fixing the issues!




Comment at: lib/Sema/SemaOverload.cpp:1146
+unsigned OldQuals = OldMethod->getTypeQualifiers().getCVRUQualifiers();
+unsigned NewQuals = NewMethod->getTypeQualifiers().getCVRUQualifiers();
 if (!getLangOpts().CPlusPlus14 && NewMethod->isConstexpr() &&

rjmccall wrote:
> This is an algorithm that I think you need to get right and which shouldn't 
> drop address spaces.
But with this patch we can only have __generic address space for methods. So if 
I try to implement something here I can't really test it? Or am I missing 
something?


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added a subscriber: akyrtzi.
rjmccall added a comment.

I have a lot of comments about various things that need to be fixed, but I just 
want to take a moment to say that this is a tremendously impressive patch: 
given only some very high-level directions, you've done a great job figuring 
out what you needed to do to carry that out and make it work.  Really well done.




Comment at: lib/AST/ASTDumper.cpp:346
+break;
+  }
   switch (EPI.RefQualifier) {

We must have some existing code to print an address space.



Comment at: lib/AST/ItaniumMangle.cpp:1507
+Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+Method->getTypeQualifiers().getCVRUQualifiers());
 // We do not consider restrict a distinguishing attribute for overloading

You can overload based on the address space, right?  I think it needs to be 
mangled.



Comment at: lib/AST/TypePrinter.cpp:804
 
-  if (unsigned quals = T->getTypeQuals()) {
+  if (unsigned quals = T->getTypeQuals().getCVRUQualifiers()) {
 OS << ' ';

You should be printing all the qualifiers here, including address spaces.  If 
you want to suppress an implicit OpenCL address-space qualifier, that seems 
reasonable.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
   getOrCreateInstanceMethodType(CGM.getContext().getPointerType(QualType(
-Ty->getClass(), FPT->getTypeQuals())),
+Ty->getClass(), 
FPT->getTypeQuals().getCVRUQualifiers())),
 FPT, U),

I think this is just `GetThisType` again.  Maybe there should be a version of 
that takes a record and an FPT; you could make it a static method on 
`CXXMethodDecl`.  At any rate, I don't think you shouldn't be dropping the 
address space.



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

Paging @akyrtzi here.  The address-space qualifier should be part of the USR 
but I don't think if there's a defined schema for that.



Comment at: lib/Parse/ParseCXXInlineMethods.cpp:419
 Sema::CXXThisScopeRAII ThisScope(Actions, Method->getParent(),
- Method->getTypeQualifiers(),
+ 
Method->getTypeQualifiers().getCVRUQualifiers(),
  getLangOpts().CPlusPlus11);

This should take the full qualifiers.  You can see how it ends up getting 
reassembled into the `this` type in the `CXXThisScopeRAII` ctor, and that type 
definitely needs to be address-space-qualified.



Comment at: lib/Sema/SemaCodeComplete.cpp:1046
+Qualifiers MethodQuals = Qualifiers::fromCVRMask(
+Method->getTypeQualifiers().getCVRQualifiers());
 if (ObjectTypeQualifiers == MethodQuals)

Hmm.  This is actually going to break code completion for these methods, I 
think: IIUC in OpenCL every pointer and l-value is address-space-qualified, so 
only considering the CVRU qualifiers means that there will always be "extra" 
qualifiers on the pointer and code completion will think it doesn't match.

...I know code completion isn't your top priority, but this is probably easy 
enough to fix.



Comment at: lib/Sema/SemaCodeComplete.cpp:3701
+  Results.setObjectTypeQualifiers(Qualifiers::fromCVRMask(
+  CurMethod->getTypeQualifiers().getCVRQualifiers()));
   MemberCompletionRecord = CurMethod->getParent();

This seems to be intentionally dropping non-CVR qualifiers, and I don't know 
why you ever would want to do that.



Comment at: lib/Sema/SemaDeclCXX.cpp:11963
+BaseType,
+CopyAssignOperator->getTypeQualifiers().getCVRUQualifiers()),
+VK_LValue, BasePath);

This should definitely not be dropping address spaces.  You'll need to make a 
second patch to handle special members correctly, so you don't need to add a 
test for this yet, but you might as well get the logic right right now.



Comment at: lib/Sema/SemaDeclCXX.cpp:12332
+BaseType,
+MoveAssignOperator->getTypeQualifiers().getCVRQualifiers()),
+VK_LValue, BasePath);

Same thing as above.



Comment at: lib/Sema/SemaOverload.cpp:1146
+unsigned OldQuals = OldMethod->getTypeQualifiers().getCVRUQualifiers();
+unsigned NewQuals = NewMethod->getTypeQualifiers().getCVRUQualifiers();
 if (!getLangOpts().CPlusPlus14 && NewMethod->isConstexpr() &&

This is an algorithm that I think you need to get right and which shouldn't 
drop address spaces.



Comment 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaType.cpp:4816
+// OpenCLCPlusPlus: A class member function has an address space.
+if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
+state.getDeclarator().getContext() ==

mikael wrote:
> Anastasia wrote:
> > Can this be moved into `deduceOpenCLImplicitAddrSpace`?
> deduceOpenCLImplicitAddrSpace is only wrapping a type with an address space. 
> I also need to make sure the FunctionProtoType gets generated with the 
> address space.
FunctionProtoType is a type too. Does it go into deduceOpenCLImplicitAddrSpace 
when creating this?


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaType.cpp:4816
+// OpenCLCPlusPlus: A class member function has an address space.
+if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
+state.getDeclarator().getContext() ==

Anastasia wrote:
> Can this be moved into `deduceOpenCLImplicitAddrSpace`?
deduceOpenCLImplicitAddrSpace is only wrapping a type with an address space. I 
also need to make sure the FunctionProtoType gets generated with the address 
space.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaType.cpp:4816
+// OpenCLCPlusPlus: A class member function has an address space.
+if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
+state.getDeclarator().getContext() ==

Can this be moved into `deduceOpenCLImplicitAddrSpace`?


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 176596.

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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZN1CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZN1CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZN1CC1Ev(%class.C addrspace(4)* %1) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZN1CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK: %3 = addrspacecast %class.C* 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/DeclCXX.h:2189
+C.addCVRUQualifiers(CVRU);
+C.addQualifiers(getType().getQualifiers());
+return C;

The address-space qualifier should get stored in the same way on the 
`FunctionProtoType` as the other method qualifiers (at least as far as the 
public interface of FPT goes).  It's not the same thing as an ordinary type 
qualifier on the type, and trying to represent it that way will lead to a lot 
of problems downstream.

You'll probably also have to do some extra work in SemaType to make sure that 
the attribute gets applied in the right place.

You'll need to mess around with the representation of FPT to make this 
efficient, because we don't want to make every instance of FPT have to pay to 
store a full `Qualifiers`.  I'd do it like this:
- Go ahead and add a full `Qualifiers` in `FunctionProtoType::ExtProtoInfo`; 
this is never stored permanently, so the cost is negligible.
- There are currently four bits for "qualifiers" in `FunctionTypeBits`.  The 
only qualifiers that are actually important to store inline like this are 
`const` and `volatile`, but since we have four bits, let's store CVR here and 
use the fourth bit to indicate that there are more qualifiers stored in 
trailing storage.
- When there are non-CVR qualifiers present, 
`ASTContext::getFunctionTypeInternal` should allocate additional trailing 
storage for a `Qualifiers` object that can store the rest of the qualifiers.
- `getTypeQuals()` can check for the bit and load the qualifiers if present, or 
else just return the CVR qualifiers stored inline.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:5254
+  if (!Context.hasSameType(From->getType(), DestType)) {
+if (From->getType().getAddressSpace() != DestType.getAddressSpace())
+  From = ImpCastExprToType(From, DestType, CK_AddressSpaceConversion,

Note that this change allowed me to revert some of the CG changes.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 176063.
Herald added subscribers: arphaman, eraman.

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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZN1CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZN1CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZN1CC1Ev(%class.C addrspace(4)* %1) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZN1CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK: %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* %3)
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:  %4 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:  %5 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:  %call1 = call 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/AST/DeclCXX.cpp:2187
   ClassTy = C.getQualifiedType(ClassTy,
Qualifiers::fromCVRUMask(getTypeQualifiers()));
+

I feel like the right design now is for `getTypeQualifiers()` to return a 
`Qualifiers` and not just a mask.

That will also help identify other places that need to be updated to support 
address-space qualification.



Comment at: lib/CodeGen/CGCall.cpp:71
 /// Derives the 'this' type for codegen purposes, i.e. ignoring method
 /// qualification.
+static CanQualType GetThisType(ASTContext , const CXXRecordDecl *RD, 
const CXXMethodDecl *MD) {

Please update the comment to say "ignoring method CVR qualification" instead.

Also, maybe we should just eliminate this and use `CXXMethodDecl::getThisType`. 
 If we want to preserve the micro-optimization of sharing `CGFunctionInfo`s for 
method types that only differ by CV qualification, maybe we should add a 
parameter to `getThisType` to not add ABI-compatible qualification.



Comment at: lib/CodeGen/CGClass.cpp:2020
+unsigned TargetAS = getContext().getTargetAddressSpace(AS);
+llvm::Type *NewType = ThisPtr->getType()->getPointerTo(TargetAS);
+ThisPtr = getTargetHooks().performAddrSpaceCast(

This is adding an extra level of pointer, I think.



Comment at: lib/CodeGen/CGClass.cpp:2023
+*this, This.getPointer(),
+getLangASFromTargetAS(This.getAddressSpace()), AS,
+NewType);

...I'm not sure when `getLangASFromTargetAS` was added, but it shouldn't exist; 
you need to pass down the language address space of `This`, which in general 
will come from the pointer type.  And then the `AS != LangAS::Default` check 
should just be comparing the constructor's address space to that address space, 
not specifically `LangAS::Default`.

Same comments apply to the code in `commonEmitCXXMemberOrOperatorCall`.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 175646.

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

https://reviews.llvm.org/D54862

Files:
  lib/AST/DeclCXX.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl

Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,140 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZN1CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZN1CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C* addrspace(4)*
+// CHECK: %1 = bitcast %class.C* addrspace(4)* %0 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %1, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %2 = addrspacecast %class.C* %c2 to %class.C* addrspace(4)*
+// CHECK:   %3 = bitcast %class.C* addrspace(4)* %2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZN1CC1Ev(%class.C addrspace(4)* %3) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %4 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %5 = addrspacecast %class.C* %c2 to %class.C* addrspace(4)*
+// CHECK:   %6 = bitcast %class.C* addrspace(4)* %5 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZN1CaSERU3AS4KS_(%class.C addrspace(4)* %6, %class.C addrspace(4)* dereferenceable(4) %4)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK: %4 = addrspacecast %class.C* %c2 to %class.C* addrspace(4)*
+// CHECK: %5 = bitcast %class.C* addrspace(4)* %4 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* %5)
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:  %6 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:  %7 = addrspacecast %class.C* %c2 to %class.C* addrspace(4)*
+// CHECK:  %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZN1CaSERU3AS4KS_(%class.C addrspace(4)* %8, %class.C addrspace(4)* dereferenceable(4) %6)
+
+TEST(__private)
+
+// CHECK-LABEL: @_Z13test__privatev
+
+// Test the address space of 'this' when invoking a method.
+// CHECK:   %2 = addrspacecast %class.C* %c to %class.C* addrspace(4)*
+// CHECK:   %3 = bitcast %class.C* addrspace(4)* %2 to %class.C addrspace(4)*
+// CHECK:   %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* %3)
+
+// Test the address space of 'this' when invoking a copy-constructor.
+// CHECK:   %5 = addrspacecast %class.C* %c to %class.C 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:80
+// used with the same version of generated operators.
+RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic);
+

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > I would suggest taking this opportunity to set up the AST to support 
> > > declaring methods in an arbitrary address space, so that you can just ask 
> > > a `CXXMethodDecl` what address space it's in.  You don't have to actually 
> > > add language support for that — OpenCL C++ would simply change the it to 
> > > the generic address space instead of the default — but I think that's the 
> > > right technical approach for implementing this, as opposed to adding a 
> > > bunch of OpenCL C++ -specific logic all over the compiler that just 
> > > hardcodes a different address space.
> > I quite like this idea. Apart from providing more clean implementation, it 
> > opens opportunities for solving several problems that I am trying to 
> > understand how to address. Specifically I am trying to find a way to 
> > 'overload' methods based on the address space of the object.
> > 
> > For example, if an object is created in the address space 1 then 
> > programmers should be able to provide a method to be used for objects in 
> > such address space for efficiency or even correctness issue.
> > 
> > The reasons I am looking at it is that currently C++ doesn't make much 
> > sense for address spaces, because we are removing them to generate just one 
> > implementation with generic/default address space. However,
> > - Not all address spaces can be converted to generic/default address space. 
> > Example in OpenCL is constant AS that can't be converted to any other.
> > - Higher performance can be achieved on some HW when using specific address 
> > spaces instead of default.
> > 
> > I was wondering if a method qualifier is a good language solution for this? 
> > For example in OpenCL we could write something like:
> > 
> >   class foo
> >   {
> >   public:
> > void bar() __private; // implies bar(__private foo*)
> > void bar() __constant; // implies bar(__constant foo*)
> >   };
> > 
> > I guess in C++ it can be done similarly:
> > 
> >   class foo
> >   {
> >   public:
> > void bar() __attribute__((address_space(1)));
> > void bar() __attribute__((address_space(2)));
> >   };
> > 
> > I would quite like to solve this generically, not just for OpenCL. I think 
> > a lot of implementation can be unified/reused then.
> > 
> > Without this address spaces seem pretty useless with C++ because they are 
> > just cast away to generic/default and no specific address space ends up at 
> > the AST level at all. This means implementation will have to rely on the 
> > optimizers to recover/deduce address spaces. But I would quite like to 
> > provide a way for the developers to manually tune the code for address 
> > spaces, just as it was done for OpenCL C.
> > 
> > Let me know if you have any thought/suggestions.
> > I was wondering if a method qualifier is a good language solution for this? 
> > For example in OpenCL we could write something like:
> 
> Yes, I think that's a very natural extension of C++'s method-qualification 
> rules for `const` and `volatile`.  Overloads would then be resolved based on 
> which address space was the best match.
> 
> Now, to briefly take a holistic perspective on the language design, this 
> feature would *strongly* benefit from a way to make a method templated over 
> the address space of `this`.  Unfortunately, I don't think that's reasonable 
> to solve in a language extension; it's really something that needs core 
> language work.  That would be a pretty big leap in scope; that said, if 
> you're interested in pursuing it, I'd be happy to share some thoughts on how 
> it'd look, and I think there are several people in the Clang community who 
> could help you with putting a proposal before the committee.
Thanks! Yes, I believe some template-based approach could make this a lot more 
usable. I would be really interested to learn more about your ideas. I would 
also quite like to see this properly in the spec. I will follow up with you in 
a separate thread.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:4035
+  V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, DestTy);
+}
 

mikael wrote:
> rjmccall wrote:
> > Always use the `performAddrSpaceConversion` target hook if there's a 
> > semantic address-space conversion required.  But really, this doesn't seem 
> > like the right place to be doing this; it ought to happen higher up when 
> > we're adding the `this` argument to the call, either explicitly in IRGen or 
> > implicitly by expecting the object expression to already yield a value in 
> > the right address space.
> > 
> > I could definitely believe that we don't currently create `CastExpr`s for 
> > simple qualification conversions of the object argument of a C++ method 
> > call, since (ignoring these address-space conversions) they're always 
> > trivial.
> Thanks for the input! It seems like the name `performAddrSpaceConversion` 
> does not exist in the code-base though.
Sorry, `performAddrSpaceCast`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGCall.cpp:4035
+  V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, DestTy);
+}
 

rjmccall wrote:
> Always use the `performAddrSpaceConversion` target hook if there's a semantic 
> address-space conversion required.  But really, this doesn't seem like the 
> right place to be doing this; it ought to happen higher up when we're adding 
> the `this` argument to the call, either explicitly in IRGen or implicitly by 
> expecting the object expression to already yield a value in the right address 
> space.
> 
> I could definitely believe that we don't currently create `CastExpr`s for 
> simple qualification conversions of the object argument of a C++ method call, 
> since (ignoring these address-space conversions) they're always trivial.
Thanks for the input! It seems like the name `performAddrSpaceConversion` does 
not exist in the code-base though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:80
+// used with the same version of generated operators.
+RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic);
+

Anastasia wrote:
> rjmccall wrote:
> > I would suggest taking this opportunity to set up the AST to support 
> > declaring methods in an arbitrary address space, so that you can just ask a 
> > `CXXMethodDecl` what address space it's in.  You don't have to actually add 
> > language support for that — OpenCL C++ would simply change the it to the 
> > generic address space instead of the default — but I think that's the right 
> > technical approach for implementing this, as opposed to adding a bunch of 
> > OpenCL C++ -specific logic all over the compiler that just hardcodes a 
> > different address space.
> I quite like this idea. Apart from providing more clean implementation, it 
> opens opportunities for solving several problems that I am trying to 
> understand how to address. Specifically I am trying to find a way to 
> 'overload' methods based on the address space of the object.
> 
> For example, if an object is created in the address space 1 then programmers 
> should be able to provide a method to be used for objects in such address 
> space for efficiency or even correctness issue.
> 
> The reasons I am looking at it is that currently C++ doesn't make much sense 
> for address spaces, because we are removing them to generate just one 
> implementation with generic/default address space. However,
> - Not all address spaces can be converted to generic/default address space. 
> Example in OpenCL is constant AS that can't be converted to any other.
> - Higher performance can be achieved on some HW when using specific address 
> spaces instead of default.
> 
> I was wondering if a method qualifier is a good language solution for this? 
> For example in OpenCL we could write something like:
> 
>   class foo
>   {
>   public:
> void bar() __private; // implies bar(__private foo*)
> void bar() __constant; // implies bar(__constant foo*)
>   };
> 
> I guess in C++ it can be done similarly:
> 
>   class foo
>   {
>   public:
> void bar() __attribute__((address_space(1)));
> void bar() __attribute__((address_space(2)));
>   };
> 
> I would quite like to solve this generically, not just for OpenCL. I think a 
> lot of implementation can be unified/reused then.
> 
> Without this address spaces seem pretty useless with C++ because they are 
> just cast away to generic/default and no specific address space ends up at 
> the AST level at all. This means implementation will have to rely on the 
> optimizers to recover/deduce address spaces. But I would quite like to 
> provide a way for the developers to manually tune the code for address 
> spaces, just as it was done for OpenCL C.
> 
> Let me know if you have any thought/suggestions.
> I was wondering if a method qualifier is a good language solution for this? 
> For example in OpenCL we could write something like:

Yes, I think that's a very natural extension of C++'s method-qualification 
rules for `const` and `volatile`.  Overloads would then be resolved based on 
which address space was the best match.

Now, to briefly take a holistic perspective on the language design, this 
feature would *strongly* benefit from a way to make a method templated over the 
address space of `this`.  Unfortunately, I don't think that's reasonable to 
solve in a language extension; it's really something that needs core language 
work.  That would be a pretty big leap in scope; that said, if you're 
interested in pursuing it, I'd be happy to share some thoughts on how it'd 
look, and I think there are several people in the Clang community who could 
help you with putting a proposal before the committee.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:80
+// used with the same version of generated operators.
+RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic);
+

rjmccall wrote:
> I would suggest taking this opportunity to set up the AST to support 
> declaring methods in an arbitrary address space, so that you can just ask a 
> `CXXMethodDecl` what address space it's in.  You don't have to actually add 
> language support for that — OpenCL C++ would simply change the it to the 
> generic address space instead of the default — but I think that's the right 
> technical approach for implementing this, as opposed to adding a bunch of 
> OpenCL C++ -specific logic all over the compiler that just hardcodes a 
> different address space.
I quite like this idea. Apart from providing more clean implementation, it 
opens opportunities for solving several problems that I am trying to understand 
how to address. Specifically I am trying to find a way to 'overload' methods 
based on the address space of the object.

For example, if an object is created in the address space 1 then programmers 
should be able to provide a method to be used for objects in such address space 
for efficiency or even correctness issue.

The reasons I am looking at it is that currently C++ doesn't make much sense 
for address spaces, because we are removing them to generate just one 
implementation with generic/default address space. However,
- Not all address spaces can be converted to generic/default address space. 
Example in OpenCL is constant AS that can't be converted to any other.
- Higher performance can be achieved on some HW when using specific address 
spaces instead of default.

I was wondering if a method qualifier is a good language solution for this? For 
example in OpenCL we could write something like:

  class foo
  {
  public:
void bar() __private; // implies bar(__private foo*)
void bar() __constant; // implies bar(__constant foo*)
  };

I guess in C++ it can be done similarly:

  class foo
  {
  public:
void bar() __attribute__((address_space(1)));
void bar() __attribute__((address_space(2)));
  };

I would quite like to solve this generically, not just for OpenCL. I think a 
lot of implementation can be unified/reused then.

Without this address spaces seem pretty useless with C++ because they are just 
cast away to generic/default and no specific address space ends up at the AST 
level at all. This means implementation will have to rely on the optimizers to 
recover/deduce address spaces. But I would quite like to provide a way for the 
developers to manually tune the code for address spaces, just as it was done 
for OpenCL C.

Let me know if you have any thought/suggestions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:80
+// used with the same version of generated operators.
+RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic);
+

I would suggest taking this opportunity to set up the AST to support declaring 
methods in an arbitrary address space, so that you can just ask a 
`CXXMethodDecl` what address space it's in.  You don't have to actually add 
language support for that — OpenCL C++ would simply change the it to the 
generic address space instead of the default — but I think that's the right 
technical approach for implementing this, as opposed to adding a bunch of 
OpenCL C++ -specific logic all over the compiler that just hardcodes a 
different address space.



Comment at: lib/CodeGen/CGCall.cpp:4035
+  V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, DestTy);
+}
 

Always use the `performAddrSpaceConversion` target hook if there's a semantic 
address-space conversion required.  But really, this doesn't seem like the 
right place to be doing this; it ought to happen higher up when we're adding 
the `this` argument to the call, either explicitly in IRGen or implicitly by 
expecting the object expression to already yield a value in the right address 
space.

I could definitely believe that we don't currently create `CastExpr`s for 
simple qualification conversions of the object argument of a C++ method call, 
since (ignoring these address-space conversions) they're always trivial.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael created this revision.
mikael added a reviewer: Anastasia.
Herald added subscribers: cfe-commits, yaxunl.

Address spaces are cast into generic before invoking the constructor.


Repository:
  rC Clang

https://reviews.llvm.org/D54862

Files:
  lib/AST/DeclCXX.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDeclCXX.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl

Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C ) { v = c.v; }
+  C =(const C ) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c2 = c;
+  C c3(c);
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZN1CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZN1CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when accessing a method.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK:   %0 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %1 = addrspacecast %class.C* %c3 to %class.C addrspace(4)*
+// CHECK:   call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %1, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+The following checkers are testing similar things like the __global test but for other address spaces.
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c2 = c;\
+C c3(c); \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+// CHECK: %2 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %2, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+// CHECK: %3 = addrspacecast %class.C* %c3 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+TEST(__private)
+
+// CHECK-LABEL: @_Z13test__privatev
+// CHECK-NOT: @__cxa_guard_acquire
+// CHECK: %0 = addrspacecast %class.C* %c to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* %0)
+// CHECK: %1 = addrspacecast %class.C* %c to %class.C addrspace(4)*
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* %1)
+// CHECK: %2 = addrspacecast %class.C* %c to %class.C addrspace(4)*
+// CHECK: %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+// CHECK: %4 = addrspacecast %class.C* %c to %class.C addrspace(4)*
+// CHECK: %5 = addrspacecast %class.C* %c3 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %5, %class.C addrspace(4)* dereferenceable(4) %4)
+
+TEST()
+
+// CHECK-LABEL: @_Z4testv()
+// CHECK: %0 = addrspacecast %class.C* %c to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* %0)
+// CHECK: %1 = addrspacecast %class.C* %c to %class.C addrspace(4)*
+// CHECK: %call = call i32