[SYCL] [NATIVECPU] Implement generic atomic store for generic target #13428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frasercrmck
reviewed
Apr 17, 2024
|
@frasercrmck I've just realized that after #13109, |
martygrant
pushed a commit
that referenced
this pull request
May 21, 2024
The previous model where we declare/define builtins based on whether target supports the generic address space was not able to capture the full complexity of what we need. We could see this in the fact that the two targets which we marked as not supporting the generic AS do in fact support it. The problem is rather that the target AS they map the generic AS to is the same target AS mapped to by the private AS. This problem hasn't properly been made apparent because all builtins we want to support with the generic AS also have overloads on the private AS. However, as can be seen in PRs like #13428, some targets want to support generic AS overloads of atomic builtins which don't also have private AS overloads. It's here that the simple dichotomy breaks down. This patch splits up the support of the generic AS into: 1. The target doesn't support the generic AS 2. The target supports the generic AS as a distinct target AS 3. The target supports the generic AS mapped identically as the private the AS All of our previous uses of case 1 have been migrated to case 3. These targets can make use of case 2 in the future to support generic AS builtins where no private AS builtins are supported. Note how we hardcode the assumption that the clash is on the private address space. This is unfortunate but for simplicity as it's the case for the two targets we care about. It was already being made, but in a less obvious way. There are ways of loosening this assumption if we ever needed to but it would currently complicate the code for untested scenarios.
|
@intel/llvm-gatekeepers this looks ready to merge, thank you |
steffenlarsen
pushed a commit
that referenced
this pull request
May 30, 2024
Following up to #13428, this PR implements the overloads for the generic AS that are missing in the `generic` libclc target.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements
__spirv_AtomicStoresimilarly to #13249. Note that theIMPLmacro has been extended to take in aSUBparameter, similarly to what happens for amdgcn and ptx.