Skip to content

Conversation

PeterTh
Copy link
Collaborator

@PeterTh PeterTh commented Aug 31, 2025

This adds support for the SYCL_KHR_WORK_ITEM_QUERIES extension, as per KhronosGroup/SYCL-Docs#682.

Although SimSYCL is a library-only implementation, due to its single-threaded nature this can be implemented via thread locals that are set every time a kernel function context is scheduled in.
The actual implementation is a bit more convoluted than I'd like, but it does provide decent error messages / developer experience.

@PeterTh PeterTh force-pushed the khr_work_item_queries branch from 92330e9 to 9342b37 Compare August 31, 2025 18:37
Copy link
Collaborator

@fknorr fknorr left a comment

Choose a reason for hiding this comment

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

I'm not entirely sure about the thread safety around the new g_ globals. We do keep a system lock around every submit, but the remaining global state in schedule.cc are thread locals (although this was the case before the system lock was introduced). If the code is sound as-is, maybe having the new variables be thread-local is still a bit easier to reason about.

@PeterTh
Copy link
Collaborator Author

PeterTh commented Aug 31, 2025

You mean as it relates to threading by the user program? In any case, I see no problem with making them thread local, so I'll do that.

Since you're here (😛), in the new unit test introduced in this PR there's this.
I adopted the whole test from here, but was surprised that it didn't actually fail when I expected it to. It looks to me like the original version never runs because the global range at dim 0 is 0, but I didn't yet check if there was just something I missed when adapting it.

@PeterTh
Copy link
Collaborator Author

PeterTh commented Sep 1, 2025

Switched to thread locals now.

@PeterTh PeterTh force-pushed the khr_work_item_queries branch from 44599fa to 81e0323 Compare September 2, 2025 10:35
Copy link
Collaborator

@fknorr fknorr left a comment

Choose a reason for hiding this comment

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

Oh, you're right about the global range, [0] being zero is not useful.

LGTM!

Comment on lines +274 to +275
const auto global_id
= range.get_offset() + (group_id * sycl::id<Dimensions>(local_range)) + local_id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a clang-format change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's how it is formatted for me right now. (I wouldn't write that on purpose ;))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants