[PATCH] D97058: [OpenCL] Refactor diagnostic for OpenCL extension/feature

Wed Mar 10 05:50:30 PST 2021

azabaznov added a comment.

> Ok, addressing in a separate patch is reasonable, but why do you think that we will break backward compatibility? My current worry is that the implementation is so messy and inconsistent that it will take us longer time if we do the incremental steps. Also, we risk introducing the regressions since it is so hard to make sense out of it.

As regarding backward compatibility -- they may be kernels which already rely on pragmas. So maybe their users already expect to see something like: //some_type requires some_ext to be enabled//. If we remove check for pragma their will be no diagnostic at all

> This issue has been reported 2 years ago but there was very little progress made since then.

I see :( Maybe we should introduce diagnostic, something like: //pragma enable is deprecated: there is no need to enable extension to use optional functionally//. But anyway it seems really impactful as current specification allows using pragma...

================
Comment at: clang/lib/Sema/Sema.cpp:339
+          setOpenCLExtensionForType(AtomicDoubleT, "cl_khr_fp64");
+          Atomic64BitTypes.push_back(AtomicDoubleT);
+        }
----------------
Anastasia wrote:
> Side comment: I don't see why would `atomic_double` have anything to do with `cl_khr_int64_base_atomics` or `cl_khr_int64_extended_atomics`? If anything the extensions should have been named differently...
Yeah... With the fact given that `atomic_int` for example by default. Intuitively I expected to see only `cl_khr_fp64` required.

OpenCL C 3.0 spec, 6.15.12.6. Atomic integer and floating-point types:
...
atomic_double [54]
...
[54]:  //The atomic_double type is only supported if double precision is supported and the cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics extensions are supported and have been enabled. If this is the case then an OpenCL C 3.0 compiler must also define the __opencl_c_fp64 feature//.

Does it seem like spec issue/inaccuracy?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97058/new/

https://reviews.llvm.org/D97058