Reapply "[libspirv] Define schar overloads via remangling; not source…
… … (#18821)"
This reapplies commit 23584c1991587815e63d95404337eb2f1faeea29. 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.
@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 :crossed_fingers: .
@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!
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.
I think the added
sycl/test/check_device_code/char_builtins.cpptest 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.
ping, thanks. I believe the Windows failure is #18892
Thanks for all the suggestions @maarquitos14 - I've applied them all :+1:
ping @intel/dpcpp-cfe-reviewers thanks