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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

@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?



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15287
+
+    if (CanQual<PointerType> ExpectedPtrTy =
+            ExpectedResultType->getAs<PointerType>()) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15289
+            ExpectedResultType->getAs<PointerType>()) {
+      ExpectedResultType = SemaRef.Context.getCanonicalType(
+          RemoveAddressSpaceFromPtr(SemaRef, ExpectedPtrTy->getTypePtr()));
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96178