Skip to content

Reapply "[libspirv] Define schar overloads via remangling; not source… #18864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 18, 2025

Conversation

frasercrmck
Copy link
Contributor

… … (#18821)"

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.

… … (intel#18821)"

This reapplies commit 23584c1. It also
includes changes from intel#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.
@frasercrmck
Copy link
Contributor Author

@hvdijk I don't suppose you're wanting to try building native-cpu SYCL-CTS with this patch? I believe it should now do the right thing 🤞 .

@hvdijk
Copy link
Contributor

hvdijk commented Jun 11, 2025

@hvdijk I don't suppose you're wanting to try building native-cpu SYCL-CTS with this patch? I believe it should now do the right thing 🤞 .

Thanks, I'll run tests tonight!

@hvdijk
Copy link
Contributor

hvdijk commented Jun 11, 2025

The testing on Native CPU completed and showed no regressions.

I think the added sycl/test/check_device_code/char_builtins.cpp test that is failing on Windows might possibly be better done as an E2E test? In its current form, it checks what the compiler emits, but it does not check that those same functions exist in libclc. As an E2E test, that would be covered, and because it would be less reliant on the exact IR that gets emitted, it would probably not give you the same problems that the current version is giving you.

@frasercrmck
Copy link
Contributor Author

I think the added sycl/test/check_device_code/char_builtins.cpp test that is failing on Windows might possibly be better done as an E2E test? In its current form, it checks what the compiler emits, but it does not check that those same functions exist in libclc. As an E2E test, that would be covered, and because it would be less reliant on the exact IR that gets emitted, it would probably not give you the same problems that the current version is giving you.

Yes you're probably right. Ideally both, to be honest. I like the idea of having quick-failing tests that show if the host mangling changes - but I'm surprised at how difficult it is to get cross-platform tests like this working (preferably with update_cc_test_checks.py to ease maintenance). I suspect some of these are just C++ things as opposed to being something SYCL is making difficult. I can't even run a check_device_code test with a windows triple because it can't find <cmath>.

But yes I've already strayed from being update_cc_test_checks.py "clean" so it might be worth biting the bullet and going all in on E2E.

@frasercrmck
Copy link
Contributor Author

ping, thanks. I believe the Windows failure is #18892

@frasercrmck
Copy link
Contributor Author

Thanks for all the suggestions @maarquitos14 - I've applied them all 👍

@frasercrmck
Copy link
Contributor Author

ping @intel/dpcpp-cfe-reviewers thanks

@ldrumm ldrumm merged commit 8f7f946 into intel:sycl Jun 18, 2025
25 checks passed
@frasercrmck frasercrmck deleted the sycl-libspirv-schar-builtins branch June 18, 2025 10:05
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.

6 participants