Rename global_mem_cache_type::write_only to read_write
#875
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
illuhad
reviewed
Nov 25, 2022
| @@ -164,7 +164,7 @@ bool cuda_hardware_context::has(device_support_aspect aspect) const { | |||
| case device_support_aspect::global_mem_cache_read_only: | |||
| return false; | |||
| break; | |||
| case device_support_aspect::global_mem_cache_write_only: | |||
| case device_support_aspect::global_mem_cache_read_write: | |||
| return false; | |||
There was a problem hiding this comment.
Now that the semantics has changed from write-only to read-write - I suspect we will have to return true for CUDA, since I think CUDA hardware has read-write cache. But better check to be sure ;)
| @@ -171,7 +171,7 @@ bool hip_hardware_context::has(device_support_aspect aspect) const { | |||
| case device_support_aspect::global_mem_cache_read_only: | |||
| return false; | |||
| break; | |||
| case device_support_aspect::global_mem_cache_write_only: | |||
| case device_support_aspect::global_mem_cache_read_write: | |||
| @@ -83,7 +83,7 @@ bool omp_hardware_context::has(device_support_aspect aspect) const { | |||
| case device_support_aspect::global_mem_cache_read_only: | |||
| return false; | |||
| break; | |||
| case device_support_aspect::global_mem_cache_write_only: | |||
| case device_support_aspect::global_mem_cache_read_write: | |||
There was a problem hiding this comment.
CPUs definitely have read-write cache
| @@ -263,7 +263,7 @@ bool ze_hardware_context::has(device_support_aspect aspect) const { | |||
| case device_support_aspect::global_mem_cache_read_only: | |||
| return false; | |||
| break; | |||
| case device_support_aspect::global_mem_cache_write_only: | |||
| case device_support_aspect::global_mem_cache_read_write: | |||
nilsfriess
commented
Dec 13, 2022
| case device_support_aspect::global_mem_cache_write_only: | ||
| return false; | ||
| case device_support_aspect::global_mem_cache_read_write: | ||
| return true; |
There was a problem hiding this comment.
I couldn't actually find a reference stating that all devices that can be used with Level Zero have read/write cache but dpc++ returns true here, and I think we can trust that this is true then.
illuhad
approved these changes
Dec 13, 2022
There was a problem hiding this comment.
From my side this looks good. Is this ready to go @nilsfriess ?
|
Yes it is :) |
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.
According to Table 25 in section A.3 of the SYCL 2020 Specification, the enum
global_mem_cache_typeshould contain the entriesnone,read_only, andread_write. The last one was calledwrite_onlyin hipSYCL, this PR renames it (and also other "linked" names to be consistent with naming).Should fix #432