[SYCL] Add property set types and JSON representation #147321
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
jhuber6
approved these changes
Jul 21, 2025
Contributor
jhuber6
left a comment
jhuber6
left a comment
There was a problem hiding this comment.
Seems reasonable, some nits.
Comment on lines
+24
to
+27
| using ByteArray = SmallVector<unsigned char, 0>; | ||
| using PropertyValue = std::variant<uint32_t, ByteArray>; | ||
| using PropertySet = std::map<std::string, PropertyValue>; | ||
| using PropertySetRegistry = std::map<std::string, PropertySet>; |
Contributor
There was a problem hiding this comment.
Can these go in the .cpp file?
Contributor
Author
There was a problem hiding this comment.
Like remove from them from the .h? These types appear in the signatures of readPropertiesFromJSON/writePropertiesFromJSON, so I don't think I should
Contributor
There was a problem hiding this comment.
I didn't see their uses, but if they're needed then it's fine.
Contributor
Author
|
@jhuber6 Thanks for the review, can the PR be merged? |
Contributor
Contributor
|
Looks okay to me. Do you want me to merge? |
Contributor
Yes. Please. Thanks |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/200/builds/14558 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/16990 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/20854 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/10696 Here is the relevant piece of the build log for the reference |
Contributor
|
Looking at the failure now. Thanks |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/21090 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/17372 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/13745 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/14836 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/22287 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/22430 Here is the relevant piece of the build log for the reference |
Contributor
|
Build failure due to "reference to local binding issue" is resolved here: #151789 Thanks |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/11801 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/8797 Here is the relevant piece of the build log for the reference |
Contributor
|
Failure due to 'undefined reference' is fixed here: #151796 Thanks |
Contributor
|
TEST 'lld :: COFF/import_weak_alias.test' failure seems unrelated. Looking at it a bit closer. Thanks |
asudarsa
added a commit
that referenced
this pull request
Aug 2, 2025
This change fixes one of the failures in #147321 Following code snippet: ` for (const auto &[CategoryName, PropSet] : PSRegistry) { J.attributeObject(CategoryName, [&] { for (const auto &[PropName, PropVal] : PropSet) { ` causes a build warning that is emitted as an error. error: reference to local binding 'PropSet' declared in enclosing lambda expression This is resolved by capturing PropSet in a local variable. Thanks Signed-off-by: Arvind Sudarsanam <[email protected]>
Contributor
Seems like this test was failing in few of the earlier PRs.. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/11248 Here is the relevant piece of the build log for the reference |
asudarsa
added a commit
that referenced
this pull request
Aug 2, 2025
This change fixes one of the failures in #147321 /usr/bin/ld: unittests/Frontend/CMakeFiles/LLVMFrontendTests.dir/PropertySetRegistryTest.cpp.o: undefined reference to symbol '_ZN4llvm10offloading21writePropertiesToJSONERKSt3mapINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES1_IS7_St7variantIJjNS_11SmallVectorIhLj0EEEEESt4lessIS7_ESaISt4pairIKS7_SB_EEESD_SaISE_ISF_SI_EEERNS_11raw_ostreamE' Need to add a missing LLVM link component in CMakeLists.txt. Thanks Signed-off-by: Arvind Sudarsanam <[email protected]>
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Aug 2, 2025
This change fixes one of the failures in llvm/llvm-project#147321 Following code snippet: ` for (const auto &[CategoryName, PropSet] : PSRegistry) { J.attributeObject(CategoryName, [&] { for (const auto &[PropName, PropVal] : PropSet) { ` causes a build warning that is emitted as an error. error: reference to local binding 'PropSet' declared in enclosing lambda expression This is resolved by capturing PropSet in a local variable. Thanks Signed-off-by: Arvind Sudarsanam <[email protected]>
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/5179 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Aug 2, 2025
…151796) This change fixes one of the failures in llvm/llvm-project#147321 /usr/bin/ld: unittests/Frontend/CMakeFiles/LLVMFrontendTests.dir/PropertySetRegistryTest.cpp.o: undefined reference to symbol '_ZN4llvm10offloading21writePropertiesToJSONERKSt3mapINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES1_IS7_St7variantIJjNS_11SmallVectorIhLj0EEEEESt4lessIS7_ESaISt4pairIKS7_SB_EEESD_SaISE_ISF_SI_EEERNS_11raw_ostreamE' Need to add a missing LLVM link component in CMakeLists.txt. Thanks Signed-off-by: Arvind Sudarsanam <[email protected]>
Contributor
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/23725 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/70/builds/10903 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/32368 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/34524 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/6/builds/10659 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the
PropertySettype, along with a pair of functions used to serialize and deserialize into a JSON representation. A property set is a key-value map, with values being one of 2 types - uint32 or byte array. A property set registry is a collection of property sets, indexed by a "category" name.In SYCL offloading, property sets will be used to communicate metadata about device images needed by the SYCL runtime. For example, there is a property set which has a byte array containing the numeric ID, offset, and size of each SYCL2020 spec constant. Another example is a property set describing the optional kernel features used in the module: does it use fp64? fp16? atomic64?
This metadata will be computed by
clang-sycl-linkerand the JSON representation will be inserted in the string table of each outputOffloadBinary. This JSON will be consumed the SYCL offload wrapper and will be lowered to the binary form SYCL runtime expects.For example, consider this SYCL program that calls a kernel that uses fp64:
The device code for this program would have the kernel marked with
!sycl_used_aspects:clang-sycl-linkerwould recognize this metadata and then would output the following JSON in theOffloadBinary's key-value map:The SYCL offload wrapper would lower those property sets to something like this: