Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SYCL][CUDA] PI API Image Support for CUDA #1954

Merged
merged 4 commits into from
Jul 8, 2020
Merged

[SYCL][CUDA] PI API Image Support for CUDA #1954

merged 4 commits into from
Jul 8, 2020

Conversation

nyalloc
Copy link
Contributor

@nyalloc nyalloc commented Jun 23, 2020 •
edited
Loading

Updates the CUDA piMem implementation to support images.

Provides implementations of the PI API piMemImageCreate, piEnqueueMemImageRead, piEnqueueMemImageWrite and piEnqueueMemImageCopy functions for CUDA backends.

The implementation required changes to the _pi_mem struct so that it supports both images and buffers, so changes were necessary across many CUDA piMem* function implementations.

Testing is provided by #1970.

@nyalloc nyalloc requested review from smaslov-intel and a team as code owners June 23, 2020 12:50
@nyalloc nyalloc changed the title {SYCL][WIP] Updated piMem implementation in preparation for images support [SYCL][WIP] Updated piMem implementation in preparation for images support Jun 23, 2020
@bader bader added the cuda CUDA back-end label Jun 23, 2020
@nyalloc nyalloc changed the title [SYCL][WIP] Updated piMem implementation in preparation for images support [SYCL][CUDA][WIP] Updated piMem implementation in preparation for images support Jun 23, 2020
@nyalloc nyalloc changed the title [SYCL][CUDA][WIP] Updated piMem implementation in preparation for images support [SYCL][CUDA] Updated piMem implementation in preparation for images support Jun 24, 2020
@nyalloc nyalloc mentioned this pull request Jun 26, 2020
Merged
@nyalloc nyalloc force-pushed the stuart/piMemUpdated branch from 3ac4ed3 to 1829803 Compare June 26, 2020 13:28
@nyalloc nyalloc changed the title [SYCL][CUDA] Updated piMem implementation in preparation for images support [SYCL][CUDA] PI API Image Support for CUDA Jun 26, 2020
@nyalloc nyalloc force-pushed the stuart/piMemUpdated branch 2 times, most recently from 207922c to 2740b19 Compare June 29, 2020 14:19
@bader
Copy link
Contributor

bader commented Jul 3, 2020

@StuartDAdams, could you fix formatting, please?

@smaslov-intel, could you take a look, please?

@nyalloc nyalloc force-pushed the stuart/piMemUpdated branch from 1c5ab05 to c3493aa Compare July 7, 2020 13:44
bader
bader previously approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smaslov-intel, @romanovvlad, could you take a look, please?

bader
bader reviewed Jul 7, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StuartDAdams, there are build failures after conflict resolution. Please, take a look.

sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved
@nyalloc
Copy link
Contributor Author

nyalloc commented Jul 8, 2020

@StuartDAdams, there are build failures after conflict resolution. Please, take a look.

Looks like git found a couple of the lines confusing when rebasing. Not sure why. Fixed now.

bader
bader previously approved these changes Jul 8, 2020
View reviewed changes
@bader
Copy link
Contributor

bader commented Jul 8, 2020

@smaslov-intel, @romanovvlad, could you take a look, please?

@nyalloc nyalloc changed the title [SYCL][CUDA] PI API Image Support for CUDA [WIP][DONOTMERGE][SYCL][CUDA] PI API Image Support for CUDA Jul 8, 2020
@romanovvlad
Copy link
Contributor

LGTM, but do we have tests on this APIs? If not, could you please add them?

@nyalloc
Copy link
Contributor Author

nyalloc commented Jul 8, 2020

LGTM, but do we have tests on this APIs? If not, could you please add them?

Testing is provided by #1970.

@nyalloc nyalloc force-pushed the stuart/piMemUpdated branch from 6a96626 to c7e76c4 Compare July 8, 2020 14:34
@nyalloc nyalloc changed the title [WIP][DONOTMERGE][SYCL][CUDA] PI API Image Support for CUDA [SYCL][CUDA] PI API Image Support for CUDA Jul 8, 2020
bader
bader previously approved these changes Jul 8, 2020
View reviewed changes
@bader bader merged commit ca67e69 into intel:sycl Jul 8, 2020
vladimirlaz
vladimirlaz reviewed Jul 5, 2021
View reviewed changes
sycl/plugins/cuda/pi_cuda.cpp
arrayDesc.Format != CU_AD_FORMAT_SIGNED_INT32 &&
arrayDesc.Format != CU_AD_FORMAT_HALF &&
arrayDesc.Format != CU_AD_FORMAT_FLOAT) {
cl::sycl::detail::pi::die(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyalloc, sorry for raising such an old PR. Could you please comment why only these four types are supported for an image. Is that the CUDA limitation or something to be fixed on plugin side?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @vladimirlaz I can answer to this, so the reason that just these image formats are supported is based on the interest in the feature at the time they were implemented, we're not aware of any limitation in CUDA that would prevent the remaining SYCL 2020 image formats (8-bit and 16-bit integer) being supported, though someone would have to investigate this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AerialMantis thank you for the info

bb-sycl pushed a commit that referenced this pull request Apr 18, 2023
@MrSidims @againull
Remove unnecessary code for joint matrix (#1954)
a0b7db0 
Signed-off-by: Sidorov, Dmitry <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@0ed2286
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladimirlaz vladimirlaz vladimirlaz left review comments

@AerialMantis AerialMantis AerialMantis left review comments

@romanovvlad romanovvlad romanovvlad approved these changes

@bader bader bader approved these changes

@smaslov-intel smaslov-intel Awaiting requested review from smaslov-intel

Assignees
No one assigned
Labels
cuda CUDA back-end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nyalloc @bader @romanovvlad @AerialMantis @vladimirlaz