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.

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

[HIP] Correctly set HIP Kernel Buffer Arguments #970

Closed

Conversation

veselypeta
Copy link
Contributor

@veselypeta veselypeta commented Oct 18, 2023

Device binary code exported from SYCL produces different interfaces depending on the backend selected. On CUDA and L0 buffer arguments are encoded with two parameters 1) A pointer to the buffer 2) a sycl::id struct (the accessor). However, on HIP the accessor argument is encoded as three individual i64 arguments.
This PR correctly sets the kernel arguments for kernels compiled and run with the HIP backend.

@kbenzie kbenzie changed the title [ADAPTERS] Fix AMD Compliation [HIP] Fix AMD Compliation Oct 18, 2023
@veselypeta veselypeta force-pushed the petr/compiler-hip-kernels branch 3 times, most recently from c9b3493 to 76e9d4b Compare October 18, 2023 12:48
@veselypeta veselypeta closed this Oct 18, 2023
@veselypeta veselypeta reopened this Oct 18, 2023
@veselypeta veselypeta force-pushed the petr/compiler-hip-kernels branch 3 times, most recently from 469fb32 to fb0d5b7 Compare October 19, 2023 14:53
@veselypeta veselypeta marked this pull request as ready for review October 19, 2023 14:55
@veselypeta veselypeta requested review from a team as code owners October 19, 2023 14:55
@ldrumm
Copy link
Contributor

ldrumm commented Oct 19, 2023

s/Compliation/Compilation/g

@veselypeta veselypeta changed the title [HIP] Fix AMD Compliation [HIP] Correctly set HIP Kernel Buffer Arguments Oct 19, 2023
Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Some changes don't look like they belong here. Please clarify their purpose, or remove unrelated cleanups

test/conformance/kernel/urKernelCreate.cpp Outdated Show resolved Hide resolved
.github/workflows/cmake.yml Outdated Show resolved Hide resolved
test/conformance/device_code/CMakeLists.txt Outdated Show resolved Hide resolved
source/adapters/hip/device.cpp Outdated Show resolved Hide resolved
@pbalcer
Copy link
Contributor

pbalcer commented Oct 20, 2023

Closes #842

@veselypeta veselypeta force-pushed the petr/compiler-hip-kernels branch 3 times, most recently from 6ee901b to a61c31f Compare October 23, 2023 09:45
@veselypeta veselypeta force-pushed the petr/compiler-hip-kernels branch 3 times, most recently from 146bfc8 to 767b1fd Compare October 23, 2023 13:38
@veselypeta veselypeta added the ready to merge Added to PR's which are ready to merge label Oct 24, 2023
@jinz2014
Copy link

@veselypeta Do you know the cause of the differences in accessor arguments between HIP and CUDA ? Thanks.

@kbenzie kbenzie added the conformance Conformance test suite issues. label Nov 2, 2023
@kbenzie kbenzie mentioned this pull request Nov 2, 2023
@veselypeta
Copy link
Contributor Author

Covered in #1030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues. ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants