Uh oh!
There was an error while loading. Please reload this page.
Add support for work_group_memory extension - #1984
Conversation
sommerlukas
commented
Feb 4, 2025
Regarding the stability of the extension: The extension was originally introduced June last year: intel/llvm#13725 Since then, one minor fix (intel/llvm#14937) and one minor API change to constructor not used in this implementation (intel/llvm#15933) were made. The implementation was added in intel/llvm#15861 in October, so should be included in the 2025.1 oneAPI releases. |
oleksandr-pavlyk
left a comment
There was a problem hiding this comment.
Thank you for your contribution @sommerlukas !
Please cite the extension document from intel/llvm or elsewhere.
It would be great to add a test exercising this kernel argument type.
Perhaps libsyclinterface/tests/test_sycl_queue_submit_local_accessor_arg could be used to add such a test.
I am happy to work with you to build the test and add it.
sommerlukas
commented
Feb 4, 2025
Thanks for your feedback @oleksandr-pavlyk!
Extension document is here. As I had discussed with @ndgrigorian offline last week, the implementation in DPCTL focuses on the variant with runtime-specified size: |
sommerlukas
commented
Feb 4, 2025
Sure, I can look into adding a test. As we would need a kernel to execute to exercise this, what's the recommended way of writing that kernel? |
oleksandr-pavlyk
commented
Feb 4, 2025
The first step would be to use DPC++, or OpenCL compiler to create SPV for a kernel that consumes Then one would need to use that SPV like it is used in the test for local_accessor argument. |
sommerlukas
commented
Feb 4, 2025
I've taken a look at The current implementation of this PR only exposes the |
ndgrigorian
commented
Feb 5, 2025
Builds seem to be failing, likely because the It would probably work best in a helper file—for an example, we use the experimental We define the macro and use the header file to indirectly include |
sommerlukas
commented
Feb 6, 2025
The use in that header isn't very idiomatic. Feature test macros for SYCL extensions are not supposed to be set by the application. Instead, the implementation should set them to a value defined in the extension specification, as described in the SYCL specification. In DPC++, this is implemented through the
If CI wants to build with the 2025.0 release compiler until a newer release comes out, I need to conditionally enable the feature based on macro and throw an exception in case somebody tries to use it with DPC++ version 2025.0 or older. |
ndgrigorian
commented
Feb 6, 2025
I see, good to know. I noticed that some of the experimental extensions don't need to be explicitly set, but Making it conditional based on compiler version would definitely be preferable. |
oleksandr-pavlyk
commented
Feb 9, 2025
This PR needs work, I suggest we take time to get it right, and it also brought to light that I have created a branch in my fork that adds changes on top of Lukas's changes in this PR (sommerlukas#1).
What is still to do:
dpctl code to build interoperability kernel bundle may need updating. |
I'm also working on improving the implementation locally, I was planning to push as soon as I have tests ready. What I've done so far:
The reason I introduced
I had considered that, but there was no precedent in the Python interface, as support for If we add this variant, IMHO it should be in addition to the current version where the number of bytes is specified. Unless we build something complicated with Cython/ctype types, we would otherwise limit the possible types of array elements to a fixed list, whereas the DPC++ extension allows user-defined types. |
oleksandr-pavlyk
commented
Feb 10, 2025
@sommerlukas The PR sommerlukas#1 was added with my changes to your branch in your fork. If you do not intend to accept it because you have already worked something out locally, please let me know, and I would break it up into smaller changes and open follow up PRs. |
sommerlukas
commented
Feb 10, 2025
Thanks for bringing that PR, I think there's some overlap with what I've done locally. Maybe it make sense to break out the changes for exposing For the changes relating to |
2356410 to
ddd8349Comparesommerlukas
commented
Feb 10, 2025
I've pushed my local changes now. The work group memory extension is now also added to I also added a test for the Python interface and a test for |
ndgrigorian
commented
Feb 10, 2025
This may make the mapping between the class and its SYCL counter-part less confusing, though there are downstream implications. |
sommerlukas
commented
Feb 10, 2025
As I mentioned offline, there's also the possibility to replace the interop to create The other benefit is that it integrates into If there's downstream implications, maybe it makes sense to build a new Extensions:
|
oleksandr-pavlyk
commented
Feb 10, 2025
My testing was done using DPC++ runtime from SYCL nightly, which may have some bugs. Using that runtime library, only |
oleksandr-pavlyk
commented
Feb 10, 2025
@sommerlukas While |
oleksandr-pavlyk
commented
Feb 10, 2025
Apart from details of the implementation suggested here, what is the idea behind exposing work-group-memory kernel argument in I feel that creating |
sommerlukas
commented
Feb 11, 2025
Yeah, this was not meant for this PR, more medium-term. Support for SPIR-V and OpenCL source languages should already be in the 2025.0 package released a few weeks ago, so 2025.0 should match DPCTL's existing support. SYCL source language is going to be in one of the other 2025.X releases soon. |
sommerlukas
commented
Feb 11, 2025
The E.g., users could compile a OpenCL kernel like this with DPCTL and pass a __kernelvoidexample_kernel(__globalfloat*input, __localfloat*local_data) {
...
}Our concrete use case is with SYCL kernels compiled via DPC++ that use |
sommerlukas
commented
Feb 11, 2025
I'm happy to move it to some other place if you and @ndgrigorian prefer. I'm also fine to not mark it experimental at all if that's preferred. |
sommerlukas
commented
Feb 11, 2025
@oleksandr-pavlyk@ndgrigorian Any hunch why the conda tests on CI fail? I can't reproduce the issue locally with my |
oleksandr-pavlyk
commented
Feb 11, 2025
Thanks for explaining, @sommerlukas , this is useful. Please add a test for OpenCL device that uses local pointer in the kernel specified in OpenCl and that is invoked using work-group-memory. |
b952f90 to
04efec6Comparesommerlukas
commented
Feb 11, 2025
I've added a test using an OpenCL kernel with |
ndgrigorian
commented
Feb 11, 2025
I think I more or less agree with @oleksandr-pavlyk , putting it aside in a special namespace which marks it experimental doesn't do much for a Python-level user—it's an implementation detail and may or may not change in the compiler in the future. If there's a check for availability already present, we can instead document that the compiler version is relevant to whether the extension works. |
diptorupd
commented
Feb 13, 2025
@oleksandr-pavlyk@sommerlukas nice set of changes. numba-dpex also supports local accessors. |
The failure looks like one I would see when building with 3.13t (free-threaded Python). Indeed, looking at the log— |
ndgrigorian
commented
Feb 21, 2025
@sommerlukas Also a quick glance at gh-1991 would be appreciated, if any of it is necessary for your work—this wasn't clear to me. |
67c915e to
2c31e1dComparesommerlukas
commented
Feb 22, 2025
Thanks @ndgrigorian! I've just rebased the PR on top of
I don't think we would use it immediately, but I will take a look early next week and review the PR. |
ndgrigorian
commented
Feb 22, 2025
This seems to have resolved most of the build issues, just the examples now need a look. |
sommerlukas
commented
Feb 25, 2025
Seems my most recent change fixed the examples, so everything but two Jenkins green now. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ndgrigorian
commented
Feb 26, 2025
No, they don't seem to be—just transient failures |
Extend kernel argument handling to add support for the work_group_memory extension, allowing users to dynamically allocate local memory for a kernel. Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>
2eaf453 to
13cbd01CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ndgrigorian
left a comment
There was a problem hiding this comment.
Besides those last nits this seems ready to go in, I'll merge after those changes are pushed
Thanks a lot @sommerlukas , and sorry it took such a long time to get it finished, getting 0.19.0 out took up a bit more time than expected.
sommerlukas
commented
Mar 7, 2025
No worries at all, thanks for your feedback @ndgrigorian! |
Extend kernel argument handling to add support for the work_group_memory extension, allowing users to dynamically allocate local memory for a kernel.