Great work! I'm a bit confused - you say in the title that you make the builtins use more overloads, but the code seems to use less overloads, and more templates instead? Or am I misunderstanding the change?

Yeah, the wording there is a bit imprecise. If you look at the 3 headers that replace the old builtin_utils, math_builtins uses (some) overloads, while generic_builtins and relational_builtins are all templates. The code duplication and repetition is sort of unfortunate, but the SYCL spec for all the categories is different enough that it's not really possible to nicely share code there.

I see. I was just asking because there was a change in the SYCL 2020 spec to define the builtins as a mixture of templates and overloads (mostly overload for float/double/half and template for marray/vec), which matters for overload resolution. But it seems your PR already takes this into account :)

To be merged after 25.02 release

I got the math builtin CTS working (for some definition of working, see this branch), but I did get the math builtins test to run. I also hacked ACPP, see this branch, but I think it works fineish.

These are the results: https://github.com/carbotaniuman/SYCL-CTS/blob/e51802a759ebda335c53b5952f41f487afe24bd2/foo

Those are sinpi and cospi like things which we fail because of precision, which I think is existing. Unfortunately this reveals that the way we handle half is just broken and blows up on a couple of edge cases. We also don't advertise fp16 support so the CTS doesn't build it, and if we enable it everything crashes terribly. It probably needs another whole refactor.

I am fairly confident that the non-half functions are correct, but I'm a bit unsure how to proceed with the half ones. I think the best way to do it would be to properly add overloads for all the builtins for half and probably to deprecate/remove the SCMP support.

I would like to get this merged, but given the absolutely shambolic half support I feel a bit uncomfortable doing so. Do you have any thoughts?

I got the math builtin CTS working (for some definition of working, see this branch), but I did get the math builtins test to run. I also hacked ACPP, see this branch, but I think it works fineish.

That's great :)

Those are sinpi and cospi like things which we fail because of precision, which I think is existing.

IMO that might be a bug in the CTS and we should consider opening an issue there. The SYCL math builtin precision is implementation-defined, so CTS would be wrong to check for a specific accuracy here. This was recently-ish clarified: KhronosGroup/SYCL-Docs#511

Unfortunately this reveals that the way we handle half is just broken and blows up on a couple of edge cases. We also don't advertise fp16 support so the CTS doesn't build it, and if we enable it everything crashes terribly. It probably needs another whole refactor. I am fairly confident that the non-half functions are correct, but I'm a bit unsure how to proceed with the half ones. I think the best way to do it would be to properly add overloads for all the builtins for half and probably to deprecate/remove the SCMP support.

Can you summarize what the issue is?

I think in the long run we probably won't get around adding half overloads to the builtin interface which the backends can then implement in whatever way makes sense for them/the target hardware. In this case, the SYCL builtin definitions in the header would just call into the builtin interface in the same way as for float or double.

We could then either implement these overloads in the builtin interface by blindly casting to float for SMCP, or implement them properly if we are more motivated.

I would like to get this merged, but given the absolutely shambolic half support I feel a bit uncomfortable doing so. Do you have any thoughts?

Even if the PR is perhaps not perfect, as far as I understand it, it doesn't make anything worse, right? As you point out we don't really advertise half. So I wouldn't be opposed to merging it. We can still improve half support in the future.

Basically the way I implemented half by delegating to double is slightly broken. After thinking about it I think it would be fine to merge it now, half is currently not that very well supported anyways and it's probably better to address that in a follow up.