[SYCL] Warn on explicit cast from default address space to named

[Fznamznon]

Such cast is allowed by the doc
https://github.com/intel/llvm/blob/sycl/clang/docs/SYCLSupport.rst#id2,
but patterns like

__global int *Global;
__private int *Private;
int *Default;

Default = Global;
Private = (__private int *)Default;

may create incorrect address space casts. It is not always that obvious where
the cast has happened and it may happen under the hood of SYCL classes like in
#3172 .
The warning provides user-friendly info about concrete place in the code
that could cause address space problems.

[Fznamznon]

Please review only last three commits, the first one is coming via llorg pulldown soon.

[premanandrao]

Could you explain this logic please?

[Fznamznon]

All named address spaces (global, local, private) are disjoint and they are sub-sets of default address space. isAddressSpaceSupersetOf function answers the question if the address space of qualifiers is a superset of argument qualifiers. We are trying to diagnose casts from default address space to named, in other words we need to diagnose when the cast happens from a big set to a small one. This check detects the case when Src pointer has address space that is a superset of Dst pointer's address space and at the same time address space of Dst pointer is not a superset of address space of Src pointer.
Also there is a verification that OpRange is valid. TryAddressSpaceCast function is also used for checks that are specifict for C++ for OpenCL and not called for SYCL, so I didn't pass the OpRange argument there, since it is not needed there anyway. But added the check to make sure that the emitted warning always has a valid source location.

[premanandrao]

... in other words we need to diagnose when the cast happens from a big set to a small one. This check detects the case when Src pointer has address space that is a superset of Dst pointer's address space and at the same time address space of Dst pointer is not a superset of address space of Src pointer.

Hi @Fznamznon, thank you, but this is the part that I would like to understand better.

Why isn't is enough to just check that 'Dst' is not a superset of 'Src'? In other words, why is 'Src' being a superset of 'Dst' checked?

I understand the OpRange part.

[Fznamznon]

Why isn't is enough to just check that 'Dst' is not a superset of 'Src'? In other words, why is 'Src' being a superset of 'Dst' checked?

Maybe it is just me being paranoid. I've changed it to check only that 'Dst' is not a superset of 'Src'. Thanks.

[Pennycook]

SYCL 2020 defines an address_space_cast function -- can we point developers to that, instead? The definition is not easy to find in the specification, so pasting it here:

template <access::address_space Space,
          access::decorated DecorateAddress,
          typename ElementType>
    multi_ptr<ElementType, Space, DecorateAddress> address_space_cast(ElementType* pointer)

Global function to create a multi_ptr instance from pointer, using the address space and decoration specified via the Space and DecorateAddress template arguments.

An implementation must return nullptr if the run-time value of pointer is not compatible with Space, and must issue a compile-time diagnostic if the deduced address space for pointer is not compatible with Space.

[Fznamznon]

I don't think that this thing is a solution. In fact, for me it seems like yet another place where we will emit incorrect address space casts when we implement it (I couldn't grep it in the headers).
It accepts the pointer in default address space. In our implementation it is always allowed to cast any pointer to default address space. multi_ptr contains a pointer to a named address space inside. That means at some point the argument pointer with default address space will be converted to a pointer with named address space, and if there was mismatch, the error will come only from SPIR-V translator or runtime, without source info, this is what people complain about. I don't know what is the compile-time diagnostic the spec mentions, but to really track the address spaces it will require some complicated analysis that is usually not implemented on FE level (and implemented by static analyzers instead).

[Pennycook]

I didn't realize that we hadn't implemented it yet. But putting that aside for now, I see address_space_cast as a solution because developers have to explicitly opt into it, and because there aren't any unsafe casts.

Unlike the Private = (__private int *)Default; example in your first post, address_space_cast<access::address_space::private>(Default) will return nullptr if the address space doesn't match. I expect the implementation will call the SPIR-V OpGenericCastToPtrExplicit instruction, so it won't look like a regular cast. So neither the SPIR-V translator nor runtime should throw an error -- it's the user's responsibility to check if the cast worked before using the result, to avoid dereferencing a null pointer. The intent is to allow developers to write code that accepts a generic pointer and which dispatches (at runtime) to address-space-specific code.

We can ignore the bit about deducing address spaces. That text only applies to SYCL implementations that cannot represent the generic address space (see here).

[Fznamznon]

I expect the implementation will call the SPIR-V OpGenericCastToPtrExplicit instruction, so it won't look like a regular cast.

Yeah, that sounds reasonable. Thanks for the explanation.

The thing is, we don't have address_space_cast implementation right now, and we still have interface of multi_ptr class that contains explicit casts between address spaces. People are complaining about address space cast errors that come without source info for a long time, so I would prefer to implement the warning while we can't provide a good solution and I'm fine to remove it once the problem is resolved.

[Pennycook]

Yeah, I agree that having the warning is going to be really helpful. And I think we should actually keep it once address_space_cast exists (but modify it to point developers towards the "safe" cast, instead).

But since we can't do anything until address_space_cast is implemented, I'm going to approve this PR and open a feature request. Sorry for holding things up!