[SYCL] Add global_device and global_host address spaces

[MrSidims]

This patch introduces 2 new address spaces in OpenCL: global_device
and global_host which are a subset of a global address space.

We want to give the user a way to tell the compiler the allocation
type of a USM pointer for optimization purposes. While it is usually
easy for our compiler to distinguish loads or stores that access local
memory from those that access global memory, distinguishing USM pointers
that access host memory from those that access device memory or even
distinguishing USM pointers that access host memory from accessors that
access global memory is currently impossible. This is because all
host code has been stripped out before we reach the backend and both
accessors and USM pointers are presenting in LLVM IR as pointers in the
global address space in the kernel's arguments. Being able to distinguish
between these types of pointers at compile time is valuable because it allows
us to instantiate simpler load-store units to perform memory transactions.

Signed-off-by: Dmitry Sidorov dmitry.sidorov@intel.com

[bader]

This patch introduces 2 new address spaces in OpenCL: global_device
and host_device which are a subset of a global address space.

host_device? You meant to say global_host? I think usm_device and usm_host names better reflect the purpose of new address spaces.

We want to give the user a way to tell the compiler the allocation
type of a USM pointer for optimization purposes. While it is usually
easy for our compiler to distinguish loads or stores that access local
memory from those that access global memory, distinguishing USM pointers
that access host memory from those that access device memory or even
distinguishing USM pointers that access host memory from accessors that
access global memory is currently impossible.

Do we really need to separate all three types of allocation or just two is enough?

This is because all
host code has been stripped out before we reach the backend and both
accessors and USM pointers are presenting in LLVM IR as pointers in the
global address space in the kernel's arguments.

This is inaccurate statement. Even if we preserve the host code it won't be possible to disambiguate the allocation type w/o additional annotations if allocation is done in separate translation unit.

Tagging @GarveyJoe.

[Naghasan]

We want to give the user a way to tell the compiler the allocation
type of a USM pointer for optimization purposes. While it is usually
easy for our compiler to distinguish loads or stores that access local
memory from those that access global memory, distinguishing USM pointers
that access host memory from those that access device memory or even
distinguishing USM pointers that access host memory from accessors that
access global memory is currently impossible.

Do we really need to separate all three types of allocation or just two is enough?

This makes global a "virtual" address space (like generic), IMO it makes sense to have the 3 of them (2 concretes + the overlapping one).

[Fznamznon]

global_device and host_device which are a subset of a global address space.

Agree with @Naghasan, I don't see the logic which makes them subset of global address space and allows conversions. Is this intention?

[Fznamznon]

It is strange that attributes named OpenCLAddressSpace... can appear only in SYCL headers. I like the idea with usm* naming.

[erichkeane]

@bader @Fznamznon : I don't know enough about the address spaces, so I hope that you and @Naghasan can make sure you review that part for me.

[MrSidims]

Thanks for the early feedback!
Some comments on general question/proposals:

1. Renaming to usm_

It indeed seems to be a better naming, will rename.

2. Do we really need to separate all three types of allocation or just two is enough?

@Naghasan is right, global AS will become kinda a "virtual" address space with one remark: at this point we don't have any plans to change the rules for OpenCL built-in types and therefore it's just another +1 to leaving global AS.

3. global to global_device and global_host (usm_device and usm_host) AS conventions

I was planning to add this functionality in a different PR, but since current implementation makes the commit message be misleading (and I don't really want to change the part, where new address spaces are claimed to be a subset of global address space), I'll bring it here as well.

4. Breaking other targets, like NVPTX

Good catch, thanks! Some thought about possibility to break existing SYCL code designs for different targets and design notes. There will be an extension in SPIR-V added that will bring SPIR-V representation of the address spaces (most likely new storage classes). In my POC I've also made a patch in clang driver, that denies support of this extension for all devices but FPGA (aka only for AOT compilation for FPGA this extension will be added to be supported, until adoption by other devices' backends). So in response for the new address spaces in SPIR-V we would see CrossWorkgroup storage class and in reversed translation we would see in LLVM IR addrspace(1) as for just global.

5. Mangling

Thanks Erich!

[MrSidims]

Addressed the comments

[erichkeane]

Other than the one change, I think this is OK. I want @Naghasan's approval first though, the details of the address space aren't as familiar to me as they seem to be to him.

[GarveyJoe]

One argument in favor of global_device/global_host instead of usm_device/usm_host is that we'd also like accessor pointers to be in the *_device address space. Thus it could be misleading if we prefix it with "usm" when pointers in that AS aren't always allocated through USM mechanisms.

[bader]

One argument in favor of global_device/global_host instead of usm_device/usm_host is that we'd also like accessor pointers to be in the *_device address space. Thus it could be misleading if we prefix it with "usm" when pointers in that AS aren't always allocated through USM mechanisms.

I'm not sure I follow this use case. @GarveyJoe, could you elaborate on that or provide a short code snippet, please?

[GarveyJoe]

I'm not sure I follow this use case. @GarveyJoe, could you elaborate on that or provide a short code snippet, please?

One motivation for adding these new address spaces is to (in a subsequent change) automatically have the frontend put all pointers that come from accessors into the device address space. This will allow backends that can exploit this information to perform additional optimization. Without this information, backends must assume that all accessor-derived pointers can point into host memory as they are in the global address space.
Of course, as @MrSidims mentioned, this will need to be done carefully so that targets that don't understand these address spaces don't choke on them. His suggestion was to only enable them to be generated if the target is AoT FPGA. I think a cleaner approach would be to always generate them from the frontend and to preserve them during IR->SPIR-V translation but to convert them to address space 1 during SPIR-V->IR translation if the given target doesn't support them. I.e. an opt-in flag would need to be passed to translator during reverse translation to preserve the new address spaces. I like this approach more because it plays nicely with JiT; in this way you don't need to know ahead of time if your target will be able to take advantage of this information.

[bader]

I'm definitely missing something. According to my understanding, the problem we are trying to solve by adding new attributes is to generate more efficient FPGA code for memory access to allocations other than "allocation on the host in USM mode". This justifies the need for usm_host attribute.
What is not clear:

• do we need differentiate "allocation on the device in USM mode" from clCreateBuffer alloation?
• Why global_device/global_host better names for new attributes than usm_device/usm_host?

[GarveyJoe]

@bader and I synched up offline. I think the part he was missing was that we are currently putting USM pointers in global and consequently they can't be distinguished from accessors.

[erichkeane]

Looking through the code, it seems OK, but I don't know enough about how we use address spaces to approve this commit. @Fznamznon and @bader will have to do that I believe.

[Fznamznon]

I'm a bit confused. It seems those address spaces are not only for USM, but there is still some USM namings.
The code overall looks ok, but it would be great if @Naghasan take a looks as well.

[Fznamznon]

Small nit, otherwise LGTM

[bader]

@Naghasan, could you take a look at updates, please?

[Naghasan]

Sorry for the late feedback.

LGTM, to avoid mangling breakage (and assert/crash) it may be good to query if it is supported by the target in Sema (so store a flag in TargetInfo and querry when parsing the address space). But maybe this can be done later.

SYCL NVPTX is not going through SPIR-V, so the round trip is not going to protect it.

[bader]

Thanks!

@MrSidims, please, update the description, so I can used it as commit message for squashed commit.

[MrSidims]

Thanks!

@MrSidims, please, update the description, so I can used it as commit message for squashed commit.

Done