Page MenuHomePhabricator

[OpenCL] Guard atomic_double with cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics
ClosedPublic

Authored by haonanya on Feb 9 2022, 6:39 PM.

Details

Summary

It is necessary to guard atomic_double type according to https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#_footnotedef_54.
Platform that disables cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics will have compiling errors even if atomic_double is not used.

Compile the following OpenCL test with options clang++ -cc1 -emit-llvm -triple spir-unknown-unknown -finclude-default-header -cl-ext=+cl_khr_fp64,+opencl_c_fp64,-cl_khr_int64_base_atomics -cl-std=CL3.0 test.cl
kernel void test_kernel1() {}

error: use of type 'atomic_double' (aka '_Atomic(double)') requires cl_khr_int64_extended_atomics extension to be enabled

Diff Detail

Event Timeline

haonanya created this revision.Feb 9 2022, 6:39 PM
haonanya requested review of this revision.Feb 9 2022, 6:39 PM
haonanya edited the summary of this revision. (Show Details)Feb 9 2022, 7:38 PM
Anastasia accepted this revision.Feb 10 2022, 4:44 AM

LGTM! Thanks

This might interfere with https://reviews.llvm.org/D119420

This revision is now accepted and ready to land.Feb 10 2022, 4:44 AM

This might interfere with https://reviews.llvm.org/D119420

Yes it will conflict.

Atomic doubles are not guarded properly for other builtins (outside of the cl_ext_float_atomics extensions) either, and I have a different solution in mind to solve it for all uses and extensions. So we could leave out the .td changes from this patch and only merge the opencl-c.h changes.

haonanya updated this revision to Diff 407506.Feb 10 2022, 5:59 AM

Remove the .td changes.

svenvh accepted this revision.Feb 10 2022, 6:20 AM

Thanks, LGTM! I'll try to followup with the .td changes soon.

Can this be committed now?

Hi, @Anastasia. Please help to land it, thanks very much.

Hi, @Anastasia. Please help to land it, thanks very much.

Sure, I will kindly ask @svenvh since he has some related patches to land too.

This revision was automatically updated to reflect the committed changes.