Skip to content

Conversation

blenderfreaky
Copy link
Contributor

@blenderfreaky blenderfreaky commented Aug 21, 2025

This PR changes the way CMake sets __SYCL_COMPILER_VERSION from using the date of compilation to:

  • using -DSYCL_COMPILER_VERSION=... if set
  • otherwise, running git to get the timestamp of the latest commit, and using that
  • falling back to the old behavior if the first two fail

See #19692 for the rationale behind this. TL;DR: Make __SYCL_COMPILER_VERSION reproducible. Closes #19692.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Whilst this is definitely better than what we have today, I would deprecate __SYCL_COMPILER_VERSION all together, simply because latest commit timestamp is not a compiler version at all.

My attempt at this is in #16137, but I'd never figured out those pre-commit failures (perhaps using WORKING_DIRECTORY instead of -- $dir is a better approach, the format is also slightly different from what I used: %cd vs %ad) and then it never left my backlog.

@blenderfreaky
Copy link
Contributor Author

blenderfreaky commented Aug 25, 2025

Working directory seems to be what's used in e.g. llvm/cmake/modules/VersionFromVCS.cmake as well.

I think %cd/committer date should be more reliable. %ad/authors date could be too old if for example a PR gets rebase onto master

Wrt. the Pre Commit issues, on your PR the logs are expired, so no idea on that end. On this one they seem(?) unrelated to the actual changes

@AlexeySachkov
Copy link
Contributor

@intel/llvm-reviewers-runtime, @againull, please share your feedback

@steffenlarsen
Copy link
Contributor

I agree that __SYCL_COMPILER_VERSION has been and with this change will continue to be a little confusing and arguably misleading. Especially since the compiler has so many release variants already.

@AlexeySachkov - Do you have any plans to revive #16137? If so, might it make sense to also add some information about the hash of the most recent commit? I don't think that should be a macro definition and it would not serve the same purpose, but while we are already looking up the most recent commit it might make sense to include as much info as possible.

Copy link
Contributor

@againull againull left a comment

Choose a reason for hiding this comment

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

I think it makes sense to merge this PR, because it aligns __SYCL_COMPILER_VERSION macro with how it is actually used in practice by users. For example, https://github.com/uxlfoundation/oneDNN/blob/main/src/gpu/intel/sycl/l0/utils.cpp#L39-L41 (if we build really old sources - without l0 support - today, then that condition will not work as expected)

Deprecation and introducing macro with the new name can be done #16137 or other follow up.
I.e. they sound like two separate changes to me.

@AlexeySachkov
Copy link
Contributor

@AlexeySachkov - Do you have any plans to revive #16137? If so, might it make sense to also add some information about the hash of the most recent commit? I don't think that should be a macro definition and it would not serve the same purpose, but while we are already looking up the most recent commit it might make sense to include as much info as possible.

I don't have immediate plans. I think that we can proceed with the current PR and deprecate the macro or do any follow-ups in a separate commit.

@blenderfreaky
Copy link
Contributor Author

The CI stuff looks unrelated to me, should I maybe rebase onto a newer commit?

Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

@againull againull merged commit 2af08ff into intel:sycl Sep 22, 2025
102 of 114 checks passed
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.

[SYCL] Make __SYCL_COMPILER_VERSION reproducible

4 participants