[SYCL][Bindless][Exp] 3D images accept 3 component vecs instead of 4 #12581
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
…nt vecs instead of 4 component vecs This commit changes read/write image functions to only accept coords with three arguments instead of the current four that is enforced. Updates tests and bindless spec to reflect this change.
DBDuncan
requested review from
a team and
sergey-semenov
and removed request for
a team
DBDuncan
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
DBDuncan
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Seanst98
approved these changes
Feb 2, 2024
DBDuncan
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
DBDuncan
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
ldrumm
requested changes
Feb 2, 2024
There was a problem hiding this comment.
I'm fine with the code changes in principal (though it's not my area of expertise so I'll let others do a more detailed review there), but I have the following requests to make this a better patchset
- The title has a typo and is long
- Your PR description (which will become the squashed commit message) should use the imperative mood e.g.
s/This commit changes/Change/(though even "change" is a bit weak because it's obviously a change or it'd be an empty commit) - I'd like some rationale as to why this change is made. Clearly you are changing from 4 element to 3, but why is that necessary?
DBDuncan
changed the title
DBDuncan
changed the title
Thanks for the feedback. Updated the title to be shorter. (and remove that spelling mistake...). Have to admit, did not think about adhering to the imperative commit message style when writing it. Will definitely do so in the future. Also, added a better explanation as to why the change was made. Made no sense to always require 4D coords to index a 3D image leading to the last coord to always never be used. Also aligns better with the rest of SYCL and other similar solutions. |
ldrumm
approved these changes
Feb 5, 2024
|
@sergey-semenov Could you please take a look? Made some changes to image.cl (As well as other changes to bindless image files.) |
sergey-semenov
approved these changes
Feb 12, 2024
|
Got approvals. @intel/llvm-gatekeepers Can this get merged? |
przemektmalon
added a commit
to codeplaysoftware/intel-llvm-mirror
that referenced
this pull request
Feb 19, 2024
- The `read_image` and `read_mipmap` APIs have been deprecated - They are replaced with the more descriptive `fetch_image`, `sample_image`, and `sample_mipmap` (for unsampled reads, sampled reads, and mipmap sampled reads, respectively). - This change is made in preperation for future functionality of fetching data from sampled images. - The reason behind this change is to avoid determining the underlying image read operation based on the coordinate type passed, and instead making it more transparent for the user which operation is performed based on the name of the function. - The extension document, bindless images headers, and all bindless images tests have all been updated. - The specification revision history has been updated to include a missed changelog entry for PR intel#12581
againull
pushed a commit
that referenced
this pull request
Feb 21, 2024
…ng (#12756) - The `read_image` and `read_mipmap` APIs have been deprecated - They are replaced with the more descriptive `fetch_image`, `sample_image`, and `sample_mipmap` (for unsampled reads, sampled reads, and mipmap sampled reads, respectively). - This change is made in preperation for future functionality of fetching data from sampled images. - The reason behind this change is to avoid determining the underlying image read operation based on the coordinate type passed, and instead making it more transparent for the user which operation is performed based on the name of the function. - The extension document, bindless images headers, and all bindless images tests have all been updated. - The specification revision history has been updated to include a missed changelog entry for PR #12581
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.
Update read/write image functions to only accept coords with three arguments instead of the current four, for 3D images.
Before this patch, when reading or writing to 3D bindless images, 4D coordinates had to be used, where the last coord is always ignored. This patch aligns bindless images to the rest of SYCL and other solutions.