Skip to content

[SYCL] Move spinlock out of sycl header tree#22270

Open
koparasy wants to merge 4 commits into
intel:syclfrom
koparasy:minor/move-spinlock-to-rt
Open

[SYCL] Move spinlock out of sycl header tree#22270
koparasy wants to merge 4 commits into
intel:syclfrom
koparasy:minor/move-spinlock-to-rt

Conversation

@koparasy

@koparasy koparasy commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Spinlock was a non-user facing header under the sycl include directory. Move to the runtime directory.

@koparasy koparasy requested a review from a team as a code owner June 9, 2026 16:38
@koparasy

koparasy commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@vmustya This relates to your work.

@koparasy

Copy link
Copy Markdown
Contributor Author

Failure is unrelated (#18552)

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. could you please elaborate on why do you need to create a separate "internal" directory for headers when we have headers in ./sycl/source/detail?
  2. could you please check that all includes of spinlock header in tools are correct. I don't see its usage in some of those files.

@koparasy

Copy link
Copy Markdown
Contributor Author
  1. could you please elaborate on why do you need to create a separate "internal" directory for headers when we have headers in ./sycl/source/detail?

Headers in source/detail are conceptually private to the runtime library. This particular header is consumed by both the RT and the tools, so leaving it in detail means the tools reach into the runtime's internals. The new directory is meant to capture that distinction: internal to the project, but with multiple in-tree consumers. This PR only moves the one header I'm touching.

I'm not claiming the boundary is fully enforced yet, but I'd rather not add to the detail cross-dependencies, and if the direction seems reasonable, other shared headers could migrate the same way over time

  1. could you please check that all includes of spinlock header in tools are correct. I don't see its usage in some of those files.

Good catch. Removed it from the ones that it is unused.

@KseniyaTikhomirova

Copy link
Copy Markdown
Contributor
  1. could you please elaborate on why do you need to create a separate "internal" directory for headers when we have headers in ./sycl/source/detail?

Headers in source/detail are conceptually private to the runtime library. This particular header is consumed by both the RT and the tools, so leaving it in detail means the tools reach into the runtime's internals. The new directory is meant to capture that distinction: internal to the project, but with multiple in-tree consumers. This PR only moves the one header I'm touching.

I'm not claiming the boundary is fully enforced yet, but I'd rather not add to the detail cross-dependencies, and if the direction seems reasonable, other shared headers could migrate the same way over time

  1. could you please check that all includes of spinlock header in tools are correct. I don't see its usage in some of those files.

Good catch. Removed it from the ones that it is unused.

thanks for the clarification.
I don't like the idea of creation of a brand new directory for this file. What was the problem with its original location? that place seems sufficient for "detail" header/classes that we want to reuse in sycl source and tools.
another possible solution can be usage of runtimes internals as it is done for this collector https://github.com/koparasy/llvm/blob/6e77ca20d3ce65864b63243768a66c02fcc1ce0f/sycl/tools/sycl-trace/CMakeLists.txt#L92
or probably the best option here is to use SpinLock implementation that is present in xpti headers (I have found it has its own impl) https://github.com/intel/llvm/blob/sycl/xpti/include/xpti/xpti_trace_framework.hpp#L351. This will eliminate this dependency from tools and SYCL SpinLock header can be safely moved to ./sycl/source/detail.

@koparasy

Copy link
Copy Markdown
Contributor Author
  1. could you please elaborate on why do you need to create a separate "internal" directory for headers when we have headers in ./sycl/source/detail?

Headers in source/detail are conceptually private to the runtime library. This particular header is consumed by both the RT and the tools, so leaving it in detail means the tools reach into the runtime's internals. The new directory is meant to capture that distinction: internal to the project, but with multiple in-tree consumers. This PR only moves the one header I'm touching.
I'm not claiming the boundary is fully enforced yet, but I'd rather not add to the detail cross-dependencies, and if the direction seems reasonable, other shared headers could migrate the same way over time

  1. could you please check that all includes of spinlock header in tools are correct. I don't see its usage in some of those files.

Good catch. Removed it from the ones that it is unused.

What was the problem with its original location?

Its original location was under sycl/include/, i.e. the installed/public tree. Everything there ends up in the user's installation. This header isn't part of the SYCL spec and, more importantly, isn't reachable from any public entry point: nothing under the public headers transitively includes it. Its use by the runtime and tools is purely an implementation detail, so it belongs in the runtime implementation/tool, not in the installed headers. That's the rule I'm applying. If something is neither specified nor reachable from a public entry point, it shouldn't live in the install directories.

https://github.com/koparasy/llvm/blob/6e77ca20d3ce65864b63243768a66c02fcc1ce0f/sycl/tools/sycl-trace/CMakeLists.txt#L92 or probably the best option here is to use SpinLock implementation that is present in xpti headers (I have found it has its own impl) https://github.com/intel/llvm/blob/sycl/xpti/include/xpti/xpti_trace_framework.hpp#L351. This will eliminate this dependency from tools and SYCL SpinLock header can be safely moved to ./sycl/source/detail.

The goal is that the directory tree tells you who consumes a header, so you don't have to read every tools' CMake to discover that dependency. detail/ reads as "runtime-private," which this isn't. That said, I don't want to turn a one-header move into a convention debate. If you'd rather it go in source/detail/ for now.

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