[SYCL][LIBCLC] Use half builtins for generic math functions

[Maetveis]

__builtin_<name>() without a suffix is equivalent to the C math function <name>(), these functions take and return double values.

Instead of using these use the f16 suffixed builtins which take and return half values.

On CPU targets these will be usually lowered to
the single precision library calls (with promotion/truncation). On other targets (for example GPUs) the half builtins might be lowered directly to hardware instructions.

Some of the builtins are not available for half precision, in these cases use the f suffixed builtins (which take and return floats).

[Maetveis]

@PietroGhg I couldn't find anything in #13829 that would suggest that using the double builtins was intentional, but LMK if I missed something.

[frasercrmck]

This seems like a good enough change for now. I'll need to inspect the LLVM IR diff for myself to be sure.

However, I think this will all have to undergo some changes in the near future. libclc shouldn't be assuming support for __builtin functions by default, unless they can be shown to be supported natively on all targets (via generic codegen expansion, for example). I'm slowly adding fully-fledged half implementations upstream, and so opting out of those by default will just leave untested code. I think targets should be opting in to builtins, having the software impementations there as the default.

As it happens, I think this only "works" for DPC++ is because we only build Native CPU, AMDGPU, and NVIDIA libclc targets (ignoring the other targets upstream supports) and because AMDGPU and NVIDIA targets override these same maths functions to use their own builtins. So only Native CPU is actually going to "see" these implementations.

If this change is in fact intended for Native CPU, then I think that target should be the one choosing to use these builtins.

But like I said, we're already in this position so this change still seems like an improvement.

[Maetveis]

However, I think this will all have to undergo some changes in the near future. libclc shouldn't be assuming support for __builtin functions by default, unless they can be shown to be supported natively on all targets (via generic codegen expansion, for example). I'm slowly adding fully-fledged half implementations upstream, and so opting out of those by default will just leave untested code. I think targets should be opting in to builtins, having the software impementations there as the default.

Agree.

As it happens, I think this only "works" for DPC++ is because we only build Native CPU, AMDGPU, and NVIDIA libclc targets (ignoring the other targets upstream supports) and because AMDGPU and NVIDIA targets override these same maths functions to use their own builtins. So only Native CPU is actually going to "see" these implementations.

If this change is in fact intended for Native CPU, then I think that target should be the one choosing to use these builtins.

This change was in-fact motivated by an out-of-tree undisclosed target, where we are (currently) still using the generic implementations. I can send you the details in some internal channel if you wish.

But like I said, we're already in this position so this change still seems like an improvement.

Yeah I figured the same myself, we could add the right builtins for the target downstream, but this felt to me like an improvement over the current state.

[frasercrmck]

As it happens, I think this only "works" for DPC++ is because we only build Native CPU, AMDGPU, and NVIDIA libclc targets (ignoring the other targets upstream supports) and because AMDGPU and NVIDIA targets override these same maths functions to use their own builtins. So only Native CPU is actually going to "see" these implementations.
If this change is in fact intended for Native CPU, then I think that target should be the one choosing to use these builtins.

This change was in-fact motivated by an out-of-tree undisclosed target, where we are (currently) still using the generic implementations. I can send you the details in some internal channel if you wish.

Ah yeah I was copied into that discussion. I didn't spot that you were in that thread too. We can take the discussion offline.

But like I said, we're already in this position so this change still seems like an improvement.

Yeah I figured the same myself, we could add the right builtins for the target downstream, but this felt to me like an improvement over the current state.

It sounds as though we'll have several cases where we want to provide both software and builtin implementations in libclc, for multiple targets each. We'll have to think of the best way to make all this code available, and have targets select between what they want. There's a few mechanisms but we'll have to find the one that suits best.

[PietroGhg]

@PietroGhg I couldn't find anything in #13829 that would suggest that using the double builtins was intentional, but LMK if I missed something.

Yeah you are right it wasn't intentional, the changes here look good to me

[Maetveis]

@jchlanda @frasercrmck I rebased and force-pushed to target sycl-web to pick up #17172

[Maetveis]

Sorry for the noise, I thought GH is not triggering GH actions for some reason, but I guess sycl-web just doesn't have any workflows configured.

[frasercrmck]

Yes, sycl-web doesn't run actions. Shame I just missed the pulldown window. I suppose if you are happy to land this on sycl-web that might make sense, but if you want it pulled down quicker maybe just merge into sycl and we can resolve the copysign conflicts as and when.

[Maetveis]

Yes, sycl-web doesn't run actions. Shame I just missed the pulldown window. I suppose if you are happy to land this on sycl-web that might make sense, but if you want it pulled down quicker maybe just merge into sycl and we can resolve the copysign conflicts as and when.

I am not in any hurry, but I'd prefer CI to run on this, so I changed it back to sycl, but dropped the copysign change. I think this should result in what we want, without conflicts when sycl-web is pulled down.

[Maetveis]

The AMD CI failure is known flaky - #17077

[Maetveis]

@intel/llvm-gatekeepers please merge :)

[steffenlarsen]

@Maetveis - parallel_for_range_roundup is failing on HIP in CI. If it is unrelated, could you please open a tracker for it and mention both the test and the tracker in a comment here?

[Maetveis]

@Maetveis - parallel_for_range_roundup is failing on HIP in CI. If it is unrelated, could you please open a tracker for it and mention both the test and the tracker in a comment here?

@steffenlarsen
I believe the error is that SYCL::Basic/parallel_for_range_roundup.cpp is flaky on AMD CI, it is already tracked by #17077.