[PATCH] D137652: Remove mandatory define of optional features macros for OpenCL C 3.0

Tue Nov 29 07:15:06 PST 2022

Anastasia added a comment.

Are these features affecting the frontend functionality in any way? If not we should implement those in the headers and if headers are not flexible enough to condition out the addition of the new macros we should extend them (potentially by extending behaviour of `-cl-ext` or introducing `undef` macros, see https://github.com/llvm/llvm-project/issues/55674). Please review the guidelines here: https://clang.llvm.org/docs/OpenCLSupport.html#opencl-extensions-and-features. The most relevant part here is:

> If an extension adds functionality that does not modify standard language parsing it should not require modifying anything other than header files and OpenCLBuiltins.td detailed in OpenCL builtins. Most commonly such extensions add functionality via libraries (by adding non-native types or functions) parsed regularly. Similar to other languages this is the most common way to add new functionality.

Overall, the idea is to avoiding modifying frontend changes if we can implement features via the headers instead. This is needed to address the scalability problems.

================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:123
+OPENCL_OPTIONALCOREFEATURE(__opencl_c_work_group_collective_functions, false, 300, OCL_C_30)
+OPENCL_OPTIONALCOREFEATURE(__opencl_c_int64, false, 300, OCL_C_30)

----------------
svenvh wrote:
> I am wondering why those features weren't added together with the other OpenCL 3.0 features; there wasn't any discussion around that in D95776.  Perhaps it's because these don't affect the compiler behaviour directly? (but then neither does e.g. `__opencl_c_atomic_order_acq_rel`)  Wondering if @Anastasia has any insights here.
Correct, I think Anton wanted to implement some diagnostics affecting `__opencl_c_atomic_order_acq_rel`, but it hasn't happened and I am not convinced it is doable. So potentially we can remove those from here in the future and migrate into the internal headers too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137652