[OpenCL] Undefine cl_intel_planar_yuv extension

[sidorovd]

Summary

Consider the code:

#pragma OPENCL EXTENSION cl_intel_planar_yuv : begin
// some declarations
#pragma OPENCL EXTENSION cl_intel_planar_yuv : end

is enough for extension to become known for clang.

Remove unnecessary definition (otherwise the extension will be define where it's not supposed to be defined).

[Anastasia]

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

Can we also test that macro my_ext is not defined here but defined above?

It seems we are not testing anything like this...

[sidorovd]

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

#pragma OPENCL EXTENSION my_ext : begin doesn't define an appropriate macro. And so cl-ext=+my_ext.

[Anastasia]

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

But don't you need to expose the definition of it?

[sidorovd]

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

Certainly I need, but now the only proper way to do so is by adding an extension via adding it in OpenCLExtensions.def. Previously we decided to avoid adding an extension directly into clang, so with a new approach I'd prefer not to add a definition of the macro in the header but define it somewhere else, otherwise the macro becomes defined where it's not supposed to be (even for ARM and AMD =) ).

[Anastasia]

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

However, my understanding is that you should define the macro when you define the extension itself.

#pragma OPENCL EXTENSION my_ext : begin
#define my_ext
...
#pragma OPENCL EXTENSION my_ext : end

does it not work for you?

[sidorovd]

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

#pragma OPENCL EXTENSION my_ext : begin
#define my_ext
...
#pragma OPENCL EXTENSION my_ext : end

^
I
It defines the macro regardless begin : end range, so one can consider that it's the same code as:

#define my_ext
#pragma OPENCL EXTENSION my_ext : begin
...
#pragma OPENCL EXTENSION my_ext : end

I agree, that it's a better way to define a macro with defining an appropriate extension itself, but it makes it be defined where it's not supposed to be (at least from my perspective).

To sum up:

1. #pragma OPENCL EXTENSION my_ext : begin ... end - makes an extension known for clang if the header included and if the extension enabled; doesn't affect a macro definition anyhow;
2. OPENCLEXT_INTERNAL(my_ext, *version*, ~0U) - makes an extension known for clang and defines an appropriate macro if the extension enabled.

I believe we are okay not to define cl_intel_planar_yuv macro in the header - just make the extension known for clang.

[Anastasia]

LGTM! I am ok with this change to remove the macro from the common header. I don't think it belongs to there. However, I believe this functionality should be better represented in the vendors specific headers rather than clang directly. See my comment above!

test/SemaOpenCL/extension-begin.cl

26 ↗

(On Diff #188332)

I believe we are okay not to define cl_intel_planar_yuv macro in the header - just make the extension known for clang.

We are not adding extensions to Clang unless they are needed for any internal functionality (i.e. to condition usage of half or double types that otherwise available in the standard C mode). If there is no special functionality needed then you should declare the macro in the header instead (probably the same place where you define the extension itself within begin/end to keep it clear). This is our official guidelines for vendor extensions currently.

cfe/trunk/lib/Headers/opencl-c.h

 
 #if __OPENCL_C_VERSION__ < CL_VERSION_2_0
 #ifdef cl_khr_3d_image_writes
 #pragma OPENCL EXTENSION cl_khr_3d_image_writes : enable
 #endif //cl_khr_3d_image_writes
 #endif //__OPENCL_C_VERSION__ < CL_VERSION_2_0
 
 #if __OPENCL_C_VERSION__ >= CL_VERSION_1_2
-#ifndef cl_intel_planar_yuv
-#define cl_intel_planar_yuv
-#endif // cl_intel_planar_yuv
 #pragma OPENCL EXTENSION cl_intel_planar_yuv : begin
 #pragma OPENCL EXTENSION cl_intel_planar_yuv : end
 #endif // __OPENCL_C_VERSION__ >= CL_VERSION_1_2
 
 #define __ovld __attribute__((overloadable))
 #define __conv __attribute__((convergent))
 
 // Optimizations

cfe/trunk/test/Headers/opencl-c-header.cl

void test_image3dwo(write_only image3d_t img) {
   write_imagef(img, (0), (0.0f));
 }
 #endif //__OPENCL_C_VERSION__
 
 // Verify that non-builtin cl_intel_planar_yuv extension is defined from
 // OpenCL 1.2 onwards.
 #if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_1_2)
 // expected-no-diagnostics
-#ifndef cl_intel_planar_yuv
-#error "Missing cl_intel_planar_yuv define"
-#endif
 #else //__OPENCL_C_VERSION__
 // expected-warning@+2{{unknown OpenCL extension 'cl_intel_planar_yuv' - ignoring}}
 #endif //__OPENCL_C_VERSION__
 #pragma OPENCL EXTENSION cl_intel_planar_yuv : enable
 
 // CHECK-MOD: Reading modules

cfe/trunk/test/SemaOpenCL/extension-begin.cl

 //
 // RUN: %clang_cc1 -cl-std=CL1.2 -DIMPLICIT_INCLUDE -include %S/extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.modules %s -verify -pedantic
 //
 // RUN: rm -rf %t.modules
 // RUN: mkdir -p %t.modules
 //
 // RUN: %clang_cc1 -cl-std=CL2.0 -DIMPLICIT_INCLUDE -include %S/extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.modules %s -verify -pedantic
 
+#pragma OPENCL EXTENSION my_ext : enable
+#ifndef IMPLICIT_INCLUDE
+// expected-warning@-2 {{unknown OpenCL extension 'my_ext' - ignoring}}
+// expected-warning@+2 {{unknown OpenCL extension 'my_ext' - ignoring}}
+#endif // IMPLICIT_INCLUDE
+#pragma OPENCL EXTENSION my_ext : disable
+
 #ifndef IMPLICIT_INCLUDE
 #include "extension-begin.h"
 #endif // IMPLICIT_INCLUDE
 #ifndef USE_PCH
 // expected-warning@extension-begin.h:4 {{expected 'disable' - ignoring}}
 // expected-warning@extension-begin.h:5 {{expected 'disable' - ignoring}}
 // expected-warning@extension-begin.h:21 {{OpenCL extension end directive mismatches begin directive - ignoring}}
 #endif // USE_PCH