[SYCL] Restore missing 'signed char' SPIR-V builtins #18807
Closed
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
A previous change 23584c1 inadvertently removed SPIR-V builtins for signed char types. Fixing this uncovered several missing builtins for regular 'char' types, too. This commit re-introduces SPIR-V builtins for all three char types where appropriate. It also fixes the codegen for several cases, as seen in the clang tests where 'char' types were previously being unnecessarily promoted to int.
frasercrmck
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
I tested NativeCPU with this PR (on commit 19b4465 to be precise) and it still fails to build SYCL-CTS. A complete list of the missing functions reported |
frasercrmck
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
frasercrmck
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Sorry about that. I believe I've fixed subsequently fixed the missing conversions. As for |
frasercrmck
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
I do see the commits that you added after your first one, but I did not think they would affect |
frasercrmck
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
frasercrmck
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Ah yep I'm also seeing I wonder if it's the |
|
That does seem to be the problem, yes: I see locally that the functions that are defined are They all have |
|
They are defined using |
|
Ah, I was checking the wrong commit, whoops. Checking on 73ff319 (just before 23584c1), I see that we were previously getting |
|
As the original was reverted, I will close this and open a new combined PR. |
ldrumm
pushed a commit
that referenced
this pull request
Jun 18, 2025
This reapplies commit 23584c1. It also includes changes from #18807 which attempt to address the issues that led to the original revert. We were previously achieving the signed char builtin definitions in libspirv via one of two ways. The first was explicitly definining schar overloads of builtins in the source. The second was by remangling 'char' builtins to one of schar or uchar, depending on the host platform. Since we are defining our builtins in OpenCL C, the plain 'char' type is already a signed type. This presents us with the opportunity to achieve our desired schar builtins solely through remangling. The primary idea is to reduce our libclc/libspirv diff with upstream. We also have the option to introduce signed char builtins upstream. As it stands the schar problem isn't far from the 'half' mangling problem that we also now deal with purely in the remangler.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A previous change 23584c1 inadvertently removed SPIR-V builtins for signed char types. Fixing this uncovered several missing builtins for regular 'char' types, too.
This commit re-introduces SPIR-V builtins for all three char types where appropriate. It also fixes the codegen for several cases, as seen in the clang tests where 'char' types were previously being unnecessarily promoted to int.