This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by azabaznov on Feb 5 2021, 1:59 PM.

Details

Summary

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

Diff Detail

Event Timeline

azabaznov created this revision.Feb 5 2021, 1:59 PM
azabaznov requested review of this revision.Feb 5 2021, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 1:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

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 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.

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.

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

Rewrote 'RemoveAddressSpaceFromPtr' to return canonical type

azabaznov marked an inline comment as done.Feb 12 2021, 11:50 AM
azabaznov added inline comments.
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 accepted this revision.Feb 15 2021, 5:53 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Feb 15 2021, 5:53 AM

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

azabaznov updated this revision to Diff 324004.Feb 16 2021, 7:05 AM

Fix clang-tidy warnings

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

This revision was landed with ongoing or failed builds.Feb 17 2021, 1:19 AM
This revision was automatically updated to reflect the committed changes.