-
Notifications
You must be signed in to change notification settings - Fork 766
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
[SYCL][CUDA] Adds PI CUDA support for reqd_work_group_size attribute #3735
Conversation
Fixes test SYCL/Basic/reqd_work_group_size.cpp in test suite for CUDA. (PR: intel/llvm-test-suite#278) |
Looks like I missed a clang driver test. Should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review for PropertySetIO
, file-table-tform
and sycl-post-link
: overall changes look good to me, just a few (mostly minor) comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation (device link and wrap image) should be updated as well. https://github.com/intel/llvm/blob/sycl/sycl/doc/images/DeviceLinkAndWrap.svg
Definitely! I have updated the documentation. Hopefully I found all the relevant sections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @kbobrovs please take a look as well.
How is this attribute supported when program is created with SPIR-V? |
I have not checked with L0, but OpenCL can both query information about PTX has In case either OpenCL or L0 may be in need of any similar program metadata, it should be as simple as just making |
Thanks, this is helpful! Both Level-Zero and OpenCL can query this information from a kernel, so we shouldn't need anything there. Since CUDA RT does not expose this information, how do you know it in JIT mode where program is created from SPIR-V, not from binary (where you add this metadata)?
Here, could we change the interface to take existing |
"
The failures are unrelated: CI issue on WIndows. |
@steffenlarsen, could you please resolve conflicts. I think everybody required reviewed and approved at least once. |
79632f4
79632f4
to
93a35ad
Compare
Thanks @pvchupin . Sorry for the double rebase. New conflicts snuck in while I was doing the first rebase. It should be ready now. |
Ouch... @steffenlarsen, sorry, I seem to introduce another merge conflict. Could you take a look, please? |
This commit adds support for reqd_work_group_size in the PI CUDA backend by extracting the attribute as program metadata. The program metadata accompanies the binary when passed to the backend and it is up to the backend if they extract any useful metadata. This adds two additional parameters to piProgramCreateWithBinary for passing the program metadata. Program metadata is transported as a properties created by sycl-post-link, so this commit also changes the behaviour of the NVPTX path for linkage actions leading to the offload wrapper. These changes uses file tables for the NVPTX path as well to allow generation and preservation of properties. This assumes that the file table only ever contains a single row if taking the NVPTX path and will fail otherwise. Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
…consistent Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>
93a35ad
to
5fda49a
Compare
Haha, no worries! It has been taken care of. Thanks for letting me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driver, doc, file-table-tform LGTM
This commit adds support for reqd_work_group_size in the PI CUDA backend
by extracting the attribute as program metadata. The program metadata
accompanies the binary when passed to the backend and it is up to the
backend if they extract any useful metadata. This adds two additional
parameters to piProgramCreateWithBinary for passing the program
metadata.
Program metadata is transported as a properties created by
sycl-post-link, so this commit also changes the behaviour of the NVPTX
path for linkage actions leading to the offload wrapper. These changes
uses file tables for the NVPTX path as well to allow generation and
preservation of properties. This assumes that the file table only ever
contains a single row if taking the NVPTX path and will fail otherwise.