[SYCL][Driver] Validate the Intel GPU names of '--offload-arch' - #23060
Draft
KornevNikita wants to merge 5 commits into
Draft
[SYCL][Driver] Validate the Intel GPU names of '--offload-arch'#23060KornevNikita wants to merge 5 commits into
KornevNikita wants to merge 5 commits into
Conversation
…ames The output of this utility is expected to be a list of names that are legal --offload-arch parameters, but for Intel GPUs it printed the name of the device, e.g. "Intel(R) Data Center GPU Max 1100", which is not one. Query the GMDID of each device with zeDeviceGetProperties and the device IP version extension, and translate its architecture and release components into an architecture name using the table in IntelGPUArch.def, which is meant to be generated from the data published by the GPU driver. A device that has no entry in the table is named after all three components of its GMDID, e.g. xe_40.11.0, so that a GPU newer than the compiler is still usable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit a7e6682f197f7603915e5de75bc17683f8be6455)
Record the IGCA level of every device in IntelGPUArch.def, so that the
table lists both names a device answers to, and add the names that cover
more than one release (xe-dg2, xe-mtl, xe-bmg, xe-ptl) with a sentinel
GMDID of zero. Move the table to clang/include/clang/Basic/ so that the
driver and the offload-arch tool can share it.
The driver now accepts, and validates against that table, the three
forms of a name the GPU driver uses: the name of an architecture
('xe-lnl-m'), the IGCA level shared by a group of architectures
('igca_40r'), and the numeric form the offload-arch tool prints for an
architecture this build has no name for ('xe_20.4.5'). Of the numeric
form only the architecture and the release are validated, as every
stepping of an architecture shares one name. Anything else is rejected
with the existing 'unsupported offload gpu architecture' diagnostic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # clang/tools/offload-arch/LevelZeroArch.cpp # clang/unittests/offload-arch/OffloadArchTest.cpp
tahonermann
reviewed
Sep 1, 2026
tahonermann
left a comment
tahonermann
left a comment
Contributor
There was a problem hiding this comment.
This looks pretty good. I added some comments regarding asserts and diagnostics.
Comment on lines
+168
to
+171
| // Only valid when isIntelCPU(), or when isIntelGPU() and !isIntelXeGPU(). | ||
| IntelArch intelKind() const { return static_cast<IntelArch>(Kind); } | ||
| // Only valid when isIntelXeGPU(); opaque outside of OffloadArch.cpp. | ||
| uint32_t intelXeKind() const { return Kind; } |
Contributor
There was a problem hiding this comment.
Any reason not to assert the preconditions?
Suggested change
| // Only valid when isIntelCPU(), or when isIntelGPU() and !isIntelXeGPU(). | |
| IntelArch intelKind() const { return static_cast<IntelArch>(Kind); } | |
| // Only valid when isIntelXeGPU(); opaque outside of OffloadArch.cpp. | |
| uint32_t intelXeKind() const { return Kind; } | |
| // Only valid when isIntelCPU(), or when isIntelGPU() and !isIntelXeGPU(). | |
| IntelArch intelKind() const { | |
| assert(isIntelCPU() || (isIntelGPU() && !isIntelXeGPU()), "Intel CPU or non-Xe GPU required"); | |
| return static_cast<IntelArch>(Kind); | |
| } | |
| // Only valid when isIntelXeGPU(); opaque outside of OffloadArch.cpp. | |
| uint32_t intelXeKind() const { | |
| assert(isIntelXeGPU(), "Intel Xe GPU required"); | |
| return Kind; | |
| } |
Comment on lines
+134
to
+136
| static constexpr OffloadArch getIntelXeGPU(uint32_t Kind) { | ||
| return {TargetArch::IntelXeGPU, Kind}; | ||
| } |
Contributor
There was a problem hiding this comment.
Can it be asserted that Kind matches a Xe GPU?
Comment on lines
+153
to
+174
| // The numeric form spells out all three components of the GMDID, but only the | ||
| // architecture and the release are validated: the revision names a stepping | ||
| // of an architecture, and a table keyed by architecture and release cannot | ||
| // tell which steppings exist. | ||
| if (!S.consume_front("xe_")) | ||
| return OffloadArch::getUnknown(); | ||
| llvm::StringRef ArchitectureStr, ReleaseStr, RevisionStr; | ||
| std::tie(ArchitectureStr, S) = S.split('.'); | ||
| std::tie(ReleaseStr, RevisionStr) = S.split('.'); | ||
| uint32_t Architecture, Release, Revision; | ||
| if (ArchitectureStr.getAsInteger(10, Architecture) || | ||
| ReleaseStr.getAsInteger(10, Release) || | ||
| RevisionStr.getAsInteger(10, Revision)) | ||
| return OffloadArch::getUnknown(); | ||
|
|
||
| for (uint32_t Index = 0; Index != NumIntelGPUArchs; ++Index) { | ||
| const IntelGPUArchEntry &Entry = IntelGPUArchs[Index]; | ||
| if (!Entry.namesAGroup() && Entry.Architecture == Architecture && | ||
| Entry.Release == Release) | ||
| return OffloadArch::getIntelXeGPU(Index); | ||
| } | ||
| return OffloadArch::getUnknown(); |
Contributor
There was a problem hiding this comment.
Shouldn't a diagnostic of some kind be emitted when the name is not recognized? Falling back to an unknown arch will, at best, result in a generic error that won't obviously map back to the unrecognized off load arch, correct?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Record the IGCA level of every device in IntelGPUArch.def, so that the
table lists both names a device answers to, and add the names that cover
more than one release (xe-dg2, xe-mtl, xe-bmg, xe-ptl) with a sentinel
GMDID of zero. Move the table to clang/include/clang/Basic/ so that the
driver and the offload-arch tool can share it.
The driver now accepts, and validates against that table, the three
forms of a name the GPU driver uses: the name of an architecture
('xe-lnl-m'), the IGCA level shared by a group of architectures
('igca_40r'), and the numeric form the offload-arch tool prints for an
architecture this build has no name for ('xe_20.4.5'). Of the numeric
form only the architecture and the release are validated, as every
stepping of an architecture shares one name. Anything else is rejected
with the existing 'unsupported offload gpu architecture' diagnostic.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com