[OpenCL] Create VoidPtrTy with generic AS in C++ for OpenCL mode

[azabaznov]

Summary

This change affects 'SemaOpenCLCXX/newdelete.cl' test,
thus the patch contains adjustments in types validation of
operators new and delete

[Anastasia]

@svenvh We haven't looked into address spaces in details when adding this. I wonder what behavior would make sense for new/delete? Do we plan to allow new/delete in named address spaces or only in generic? It might be more helpful to allow named address spaces since generic has not been intended for allocations, but however generic makes things easier because it avoids duplication and the concrete address space can always be cast to it. I think constant doesn't make sense at all since nothing can be done with a readonly chunk of memory?

Another aspect to consider - how will address spaces aligned with the concept of overloading i.e.

• Declaring multiple new operators with different address spaces in the same scope generates an error since overloading by the return type is not allowed.
• Declaring multiple delete operators differing by address spaces seems to fail because C++ doesn't allow to overload it yet.

While the test in the review works, if we start using other than generic address spaces it fails with various compile-time errors. For example, I get an error if I try to assign to local address space pointer even if the declaration of new has the address space too:

`class A {
public:

__local void* operator new(size_t);

};
__local A* a = new A;`

error: assigning 'A *' to '__local A *' changes address space of pointer

Perhaps this deserves a PR at least?

clang/lib/Sema/SemaDeclCXX.cpp

15289

I think getAs returns a PointerType* that we could just use straight away without the need for CanQual? but actually, you could just change to auto* that would make things easier.

The same below.

15291

FYI I think ExpectedResultType is going to be in always in the canonical form here by the way it is constructed so getCanonicalType should be redundant.

[azabaznov]

Thanks for review. Just to be clear: can I proceed with this? Or we should discuss it first? As I see possible this change can be abandoned after discussion.

Moreover,  I see nothing about address space of void pointer in OpenCL C spec. For example (OpenCL C 2.2, Clause 6.3.2.3 - Pointers, replace the first two paragraphs with the following paragraphs):

A pointer to void in any address space may be converted to or from a pointer to any incomplete or
object type. A pointer to any incomplete or object type in some address space may be converted to a
pointer to void in an enclosing address space and back again; the result shall compare equal to the
original pointer.

Do I understand correctly that this means void pointer is not necessarily points to generic AS?

[svenvh]

In D96178#2551449, @Anastasia wrote:

@svenvh We haven't looked into address spaces in details when adding this. I wonder what behavior would make sense for new/delete? Do we plan to allow new/delete in named address spaces or only in generic? It might be more helpful to allow named address spaces since generic has not been intended for allocations, but however generic makes things easier because it avoids duplication and the concrete address space can always be cast to it. I think constant doesn't make sense at all since nothing can be done with a readonly chunk of memory?

It seems the use of new/delete still needs a more thorough investigation indeed. I'd suggest we don't try to solve that in this patch (nor hold this back), so a PR sounds like a good option.

[Anastasia]

Do I understand correctly that this means void pointer is not necessarily points to generic AS?

This is an internal Clang pointer type that is used in very few places as a pointer to anything, so it makes sense to add generic.

I agree this change actually fixes a bug i.e. missing generic address space for C++ mode. We should go ahead and commit it.

[Anastasia]

PS, I will create a PR to follow up on the issue I have discovered.

[azabaznov]

Rewrote 'RemoveAddressSpaceFromPtr' to return canonical type

[azabaznov]

clang/lib/Sema/SemaDeclCXX.cpp

15291

Not sure if I understand this one, because QualType can't be cast to CanQualType so I think getCanonicalType() is needed. But I tried to unify existing logic and rewrote RemoveAddressSpaceFromPtr() to return CanQualType as it can be implicitly cast to QualType.

[Anastasia]

LGTM! Thanks!

[Anastasia]

FYI I have created PR49191 for the issues with address spaces I have highlighted earlier.

[azabaznov]

Fix clang-tidy warnings

[azabaznov]

Thanks for review. Last change is a small fix for if clang-tidy warnings

clang/lib/AST/ASTContext.cpp

#include "clang/Basic/PPCTypes.def"
   ObjCBuiltinBoolTy = (Target.useSignedCharForObjCBool() ?
                        SignedCharTy : BoolTy);
 
   ObjCConstantStringType = QualType();
 
   ObjCSuperType = QualType();
 
   // void * type
-  if (LangOpts.OpenCLVersion >= 200) {
+  if (LangOpts.OpenCLGenericAddressSpace) {
     auto Q = VoidTy.getQualifiers();
     Q.setAddressSpace(LangAS::opencl_generic);
     VoidPtrTy = getPointerType(getCanonicalType(
         getQualifiedType(VoidTy.getUnqualifiedType(), Q)));
   } else {
     VoidPtrTy = getPointerType(VoidTy);
   }
 

clang/lib/Sema/SemaDeclCXX.cpp

if (isa<TranslationUnitDecl>(DC) &&
     return SemaRef.Diag(FnDecl->getLocation(),
                         diag::err_operator_new_delete_declared_static)
       << FnDecl->getDeclName();
   }
 
   return false;
 }
 
-static QualType
-RemoveAddressSpaceFromPtr(Sema &SemaRef, const PointerType *PtrTy) {
-  QualType QTy = PtrTy->getPointeeType();
-  QTy = SemaRef.Context.removeAddrSpaceQualType(QTy);
-  return SemaRef.Context.getPointerType(QTy);
+static CanQualType RemoveAddressSpaceFromPtr(Sema &SemaRef,
+                                             const PointerType *PtrTy) {
+  auto &Ctx = SemaRef.Context;
+  Qualifiers PtrQuals = PtrTy->getPointeeType().getQualifiers();
+  PtrQuals.removeAddressSpace();
+  return Ctx.getPointerType(Ctx.getCanonicalType(Ctx.getQualifiedType(
+      PtrTy->getPointeeType().getUnqualifiedType(), PtrQuals)));
 }
 
 static inline bool
 CheckOperatorNewDeleteTypes(Sema &SemaRef, const FunctionDecl *FnDecl,
                             CanQualType ExpectedResultType,
                             CanQualType ExpectedFirstParamType,
                             unsigned DependentParamTypeDiag,
                             unsigned InvalidParamTypeDiag) {
   QualType ResultType =
       FnDecl->getType()->castAs<FunctionType>()->getReturnType();
 
-  // The operator is valid on any address space for OpenCL.
   if (SemaRef.getLangOpts().OpenCLCPlusPlus) {
-    if (auto *PtrTy = ResultType->getAs<PointerType>()) {
+    // The operator is valid on any address space for OpenCL.
+    // Drop address space from actual and expected result types.
+    if (const auto *PtrTy = ResultType->getAs<PointerType>())
       ResultType = RemoveAddressSpaceFromPtr(SemaRef, PtrTy);
-    }
+
+    if (auto ExpectedPtrTy = ExpectedResultType->getAs<PointerType>())
+      ExpectedResultType = RemoveAddressSpaceFromPtr(SemaRef, ExpectedPtrTy);
   }
 
   // Check that the result type is what we expect.
   if (SemaRef.Context.getCanonicalType(ResultType) != ExpectedResultType) {
     // Reject even if the type is dependent; an operator delete function is
     // required to have a non-dependent result type.
     return SemaRef.Diag(
                FnDecl->getLocation(),
                ResultType->isDependentType()
                    ? diag::err_operator_new_delete_dependent_result_type
CheckOperatorNewDeleteTypes(Sema &SemaRef, const FunctionDecl *FnDecl,
   if (FnDecl->getNumParams() == 0)
     return SemaRef.Diag(FnDecl->getLocation(),
                         diag::err_operator_new_delete_too_few_parameters)
       << FnDecl->getDeclName();
 
   QualType FirstParamType = FnDecl->getParamDecl(0)->getType();
   if (SemaRef.getLangOpts().OpenCLCPlusPlus) {
     // The operator is valid on any address space for OpenCL.
-    if (auto *PtrTy =
-            FnDecl->getParamDecl(0)->getType()->getAs<PointerType>()) {
+    // Drop address space from actual and expected first parameter types.
+    if (const auto *PtrTy =
+            FnDecl->getParamDecl(0)->getType()->getAs<PointerType>())
       FirstParamType = RemoveAddressSpaceFromPtr(SemaRef, PtrTy);
-    }
+
+    if (auto ExpectedPtrTy = ExpectedFirstParamType->getAs<PointerType>())
+      ExpectedFirstParamType =
+          RemoveAddressSpaceFromPtr(SemaRef, ExpectedPtrTy);
   }
 
   // Check that the first parameter type is what we expect.
   if (SemaRef.Context.getCanonicalType(FirstParamType).getUnqualifiedType() !=
       ExpectedFirstParamType) {
     // The first parameter type is not allowed to be dependent. As a tentative
     // DR resolution, we allow a dependent parameter type if it is the right
     // type anyway, to allow destroying operator delete in class templates.

clang/test/CodeGenOpenCLCXX/addrspace-new-delete.cl

+// RUN: %clang_cc1 %s -triple spir -cl-std=clc++ -emit-llvm -O0 -o - | FileCheck %s
+
+typedef __SIZE_TYPE__ size_t;
+
+class A {
+public:
+  void* operator new(size_t);
+  void operator delete(void *ptr);
+};
+
+void test_new_delete(A **a) {
+// CHECK: %{{.*}} = call spir_func i8 addrspace(4)* @_ZNU3AS41AnwEj(i32 {{.*}})
+    *a = new A;
+// CHECK: call spir_func void @_ZNU3AS41AdlEPU3AS4v(i8 addrspace(4)* {{.*}})
+    delete *a;
+}