llvm icon indicating copy to clipboard operation
llvm copied to clipboard

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

Open frasercrmck opened this issue 7 months ago • 6 comments

… … (#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.

frasercrmck avatar Jun 09 '25 16:06 frasercrmck

@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: .

frasercrmck avatar Jun 11 '25 10:06 frasercrmck

@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 avatar Jun 11 '25 11:06 hvdijk

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.

hvdijk avatar Jun 11 '25 21:06 hvdijk

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 avatar Jun 12 '25 09:06 frasercrmck

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

frasercrmck avatar Jun 16 '25 09:06 frasercrmck

Thanks for all the suggestions @maarquitos14 - I've applied them all :+1:

frasercrmck avatar Jun 16 '25 16:06 frasercrmck

ping @intel/dpcpp-cfe-reviewers thanks

frasercrmck avatar Jun 18 '25 08:06 frasercrmck