Closed lakshmih closed 2 weeks ago
An alternative (better?) fix could be to declare
source
ashalf *
, as the test is castingsource
tohalf *
anyway.
My gut feeling is that this would be slightly better, but I honestly don't know what these half
tests are trying to do. There seems to be an assumption in the tests that using a CL_HALF_FLOAT
channel data type requires the input data to be half
also, but this isn't the case and we could use float
data just as well (indeed, the tests end up calling the same write_imagef
for these images).
So, I guess my preference would be to tear out all of this unnecessary half
stuff and just use float
instead: this would simplify the tests and it would also allow the CL_HALF_FLOAT
channel data type - which is a core channel data type and not part of the cl_khr_fp16
extension! - to be tested on more devices. I can certainly appreciate that this is bigger change, though.
Maybe we should do this:
cl_khr_fp16
for these cases, and the kernels are using half4
, I'd be fine with this PR as-is.cl_khr_fp16
and ideally fix the incorrect test assumptions. Good: remove the checks for cl_khr_fp16
, the changes in this PR to enable cl_khr_fp16
, and use half*
instead of half4*
. Better: remove all of the half
kernels and use of the half
type, also.half
types require cl_khr_fp16
and which do not. I think you're right and half4
requires cl_khr_fp16
, but this could be described more clearly, especially now that the extensions are described in the main spec.FWIW, the latest upstream Clang isn't behaving the way I'd expect it to, especially for half4
. See: https://godbolt.org/z/M67s8rfeK
FWIW, the latest upstream Clang isn't behaving the way I'd expect it to, especially for
half4
. See: https://godbolt.org/z/M67s8rfeK
Nice find, from a quick look through the clang source code this must have been present for a long time. Assuming we want to diagnose this, feel free to raise an issue in https://github.com/llvm/llvm-project/issues and assign it to me.
Yes upstream clang seems to be compiling this successfully however the spec does seem to require cl_khr_fp16 for functions taking or returning half types as per https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#vector-data-load-and-store-functions.
| All functions taking or returning half types are supported only when the cl_khr_fp16 extension macro is supported.|
Can we merge this change and tackle any spec modifications separately. Thanks.
FWIW, the latest upstream Clang isn't behaving the way I'd expect it to, especially for
half4
. See: https://godbolt.org/z/M67s8rfeK
I've opened https://github.com/llvm/llvm-project/pull/96640 to fix the issue of allowing halfn types while the cl_khr_fp16 extension isn't enabled.
I was confused about this as well initially, but the specification for
halfn
has a footnote that states cl_khr_fp16 must be enabled; i.e., the test cannot usehalf4
without also enabling cl_khr_fp16. The scalarhalf
type does not have such a footnote.An alternative (better?) fix could be to declare
source
ashalf *
, as the test is castingsource
tohalf *
anyway.