[SYCL][libclc][NATIVECPU] Implement generic atomic load for generic target #13249

PietroGhg · 2024-04-02T09:07:15Z

This PR implements the overload for the generic address space for __spirv_AtomicLoad in the generic target.

Libclc implements overloads for the generic address space for __spirv_AtomicLoad (and several other builtins) in the ptx and amdgcn targets, but doesn't do so for generic.

I've created this PR to gather some initial feedback on the implementation, I'd like to add implementations for other builtins with follow up PRs.

hdelan · 2024-04-02T14:38:07Z

Is it safe to have explicit mangling for libclc/generic with AS ptr params? As far as I am aware, the mangling of generic AS pointers can depend on the target that it is being compiled for. This patch does not add builtins for the generic AS, so maybe this is not a concern. My main question is: do we have guarantees that the mangling of global and local AS ptrs will always be the same, regardless of the target being compiled for? I think the answer is yes, but would like to confirm.

Tagging @frasercrmck for context

PietroGhg · 2024-04-02T14:55:59Z

Is it safe to have explicit mangling for libclc/generic with AS ptr params? As far as I am aware, the mangling of generic AS pointers can depend on the target that it is being compiled for. This patch does not add builtins for the generic AS, so maybe this is not a concern. My main question is: do we have guarantees that the mangling of global and local AS ptrs will always be the same, regardless of the target being compiled for? I think the answer is yes, but would like to confirm.

Tagging @frasercrmck for context

Thanks @hdelan, you raise a good point. My understanding is that in general mangling is target dependent, and in particular when it comes to address spaces, the mangling depends in the Address Space Map used by the compiler front end, and this applies to all address spaces, not just the generic one. So yeah here I think we are assuming that the generic AS is mapped to 0 when mangling, which is probably not too different from other assumptions that are already made in libclc (global->1, local->3) ?

frasercrmck · 2024-04-02T15:20:28Z

Is it safe to have explicit mangling for libclc/generic with AS ptr params? As far as I am aware, the mangling of generic AS pointers can depend on the target that it is being compiled for. This patch does not add builtins for the generic AS, so maybe this is not a concern. My main question is: do we have guarantees that the mangling of global and local AS ptrs will always be the same, regardless of the target being compiled for? I think the answer is yes, but would like to confirm.
Tagging @frasercrmck for context

Thanks @hdelan, you raise a good point. My understanding is that in general mangling is target dependent, and in particular when it comes to address spaces, the mangling depends in the Address Space Map used by the compiler front end, and this applies to all address spaces, not just the generic one. So yeah here I think we are assuming that the generic AS is mapped to 0 when mangling, which is probably not too different from other assumptions that are already made in libclc (global->1, local->3) ?

Yeah, it's a bit tricky.

All manglings are ultimately up to the target, and may depend on other options (see AMDGPU changing the address space mapping depending on whether -x cl is passed). It's quite lucky that we happen to have global and local (and constant so stable) but generic doesn't adhere to this. It's commonly 0, but may be 5 on AMDGPU. Also, the target address space isn't the whole story, because address space 0 isn't universally mangled as P - it could be PU3AS0, if the default address space isn't zero (again, see AMDGPU).

We also have the problem in libclc where manglings need to be stable across different targets (e.g., the remangling process) such as ensuring address space manglings line up between "host" and "device" code.

I don't have a solid recommendation, but the generic address space can be mangled in three different ways on AMDGPU (P, PU3AS0, PU3AS5), and we currently make use of two of those (P, PU3AS0), and have a murky (buggy?) remangling process which happens to fudge those into just the one (PU3AS0 -> P, which is what the host expects).

My gut feeling is that we shouldn't have any hard-coded address space manglings in the generic code. But that's too late as we already have those! So maybe this is okay, or we can hack it with CMake (which is just replicating clang knowledge in a hacky way, so I'm not happy about it either).

frasercrmck · 2024-04-02T15:20:32Z

libclc/generic/libspirv/atomic/atomic_load.cl


 IMPL_AS(int, i, , 4)
 IMPL_AS(unsigned int, j, u, 4)

+IMPL(unsigned int, j, global, , u, 4)


Should this really be global? I thought the PR was for generic AS.

Thanks for spotting it, I copy-pasted the line and didn't notice it. I changed to a blank in the call to the macro since OpenCL 3.0 doesn't allot using generic as an AS qualifier, I've also added implementations of the clc* load helpers that use addrspace(0).

frasercrmck

LGTM with nits. As discussed I'm not a fan of manual mangling (especially in the generic builtins) but we are already in that situation, so this PR isn't making anything worse. We'll investigate ways of improving this, I think.

frasercrmck · 2024-04-10T09:27:25Z

libclc/generic/libspirv/atomic/atomic_load.cl

@@ -18,7 +18,7 @@ TYPE __clc__atomic_##PREFIX##load_##AS##_##BYTE_SIZE##_##MEM_ORDER(volatile AS c
  FDECL(TYPE, PREFIX, AS, BYTE_SIZE, acquire)                                                                     \
  FDECL(TYPE, PREFIX, AS, BYTE_SIZE, seq_cst)                                                                     \
  _CLC_DEF TYPE                                                                                                   \
-      _Z18__spirv_AtomicLoadPU3##AS_MANGLED##K##TYPE_MANGLED##N5__spv5Scope4FlagENS1_19MemorySemanticsMask4FlagE( \
+      _Z18__spirv_AtomicLoadP##AS_MANGLED##K##TYPE_MANGLED##N5__spv5Scope4FlagENS1_19MemorySemanticsMask4FlagE( \


Formatting looks like it could be improved - that \ should be aligned with the others

I've aligned the \ at the end of the lines, thank you

frasercrmck · 2024-04-10T09:27:47Z

libclc/generic/libspirv/atomic/atomic_load.cl

@@ -31,8 +31,9 @@ TYPE __clc__atomic_##PREFIX##load_##AS##_##BYTE_SIZE##_##MEM_ORDER(volatile AS c
  }

 #define IMPL_AS(TYPE, TYPE_MANGLED, PREFIX, BYTE_SIZE) \


Are you able to format this whole expression while you're here? It'd be nice to slowly improve the formatting of libclc as we go.

PietroGhg · 2024-04-10T13:26:44Z

@intel/llvm-gatekeepers this looks ready to be merged, thank you

…13428) Implements `__spirv_AtomicStore` similarly to #13249. Note that the `IMPL` macro has been extended to take in a `SUB` parameter, similarly to what happens for [amdgcn](https://github.com/intel/llvm/blob/a5a0e1296269195de90949537597b2788bb5e836/libclc/amdgcn-amdhsa/libspirv/atomic/atomic_store.cl#L13) and [ptx](https://github.com/intel/llvm/blob/a5a0e1296269195de90949537597b2788bb5e836/libclc/ptx-nvidiacl/libspirv/atomic/atomic_store.cl#L39).

PietroGhg added 4 commits March 18, 2024 16:39

[wip] no addrspace in generic libclc

3427c73

Merge branch 'sycl' into pietro/atomic_load

47bc9ff

Merge branch 'sycl' into pietro/atomic_load

6c9ab5b

Add impl for int

f64cdae

PietroGhg requested a review from a team as a code owner April 2, 2024 09:07

PietroGhg requested a review from hdelan April 2, 2024 09:07

PietroGhg had a problem deploying to WindowsCILock April 2, 2024 09:07 — with GitHub Actions Error

add prefix in mangled as

67c002f

PietroGhg temporarily deployed to WindowsCILock April 2, 2024 09:22 — with GitHub Actions Inactive

PietroGhg temporarily deployed to WindowsCILock April 2, 2024 09:51 — with GitHub Actions Inactive

frasercrmck reviewed Apr 2, 2024

View reviewed changes

use generic as in impl

f236865

PietroGhg temporarily deployed to WindowsCILock April 3, 2024 08:36 — with GitHub Actions Inactive

PietroGhg temporarily deployed to WindowsCILock April 3, 2024 08:59 — with GitHub Actions Inactive

frasercrmck approved these changes Apr 10, 2024

View reviewed changes

formatting

a0c112d

PietroGhg temporarily deployed to WindowsCILock April 10, 2024 09:56 — with GitHub Actions Inactive

PietroGhg temporarily deployed to WindowsCILock April 10, 2024 10:17 — with GitHub Actions Inactive

dm-vodopyanov merged commit 65bdffb into intel:sycl Apr 10, 2024

PietroGhg mentioned this pull request Apr 16, 2024

[SYCL] [NATIVECPU] Implement generic atomic store for generic target #13428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL][libclc][NATIVECPU] Implement generic atomic load for generic target #13249

[SYCL][libclc][NATIVECPU] Implement generic atomic load for generic target #13249

Uh oh!

PietroGhg commented Apr 2, 2024

Uh oh!

hdelan commented Apr 2, 2024

Uh oh!

PietroGhg commented Apr 2, 2024

Uh oh!

frasercrmck commented Apr 2, 2024

Uh oh!

frasercrmck Apr 2, 2024

Uh oh!

PietroGhg Apr 3, 2024

Uh oh!

frasercrmck left a comment

Uh oh!

frasercrmck Apr 10, 2024

Uh oh!

PietroGhg Apr 10, 2024

Uh oh!

frasercrmck Apr 10, 2024

Uh oh!

PietroGhg Apr 10, 2024

Uh oh!

PietroGhg commented Apr 10, 2024

Uh oh!

Uh oh!

		@@ -31,8 +31,9 @@ TYPE __clc__atomic_##PREFIX##load_##AS##_##BYTE_SIZE##_##MEM_ORDER(volatile AS c
		}

		#define IMPL_AS(TYPE, TYPE_MANGLED, PREFIX, BYTE_SIZE) \

[SYCL][libclc][NATIVECPU] Implement generic atomic load for generic target #13249

[SYCL][libclc][NATIVECPU] Implement generic atomic load for generic target #13249

Uh oh!

Conversation

PietroGhg commented Apr 2, 2024

Uh oh!

hdelan commented Apr 2, 2024

Uh oh!

PietroGhg commented Apr 2, 2024

Uh oh!

frasercrmck commented Apr 2, 2024

Uh oh!

frasercrmck Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

PietroGhg Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

frasercrmck left a comment

Choose a reason for hiding this comment

Uh oh!

frasercrmck Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

PietroGhg Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

frasercrmck Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

PietroGhg Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

PietroGhg commented Apr 10, 2024

Uh oh!

Uh oh!