[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa779a169931c: [OpenCL] Remove unused extensions (authored by 
mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,16 +34,6 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
 #ifndef cl_khr_byte_addressable_store
@@ -86,15 +76,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,24 +94,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
 #ifndef cles_khr_int64
 #error "Missing cles_khr_int64 define"
@@ -140,60 +103,6 @@
 #endif
 #pragma OPENCL EXTENSION cles_khr_int64 : enable
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_image2d_from_buffer : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_initialize_memory
-#error "Missing cl_khr_initialize_memory define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_initialize_memory' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_initialize_memory : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_gl_depth_images
-#error "Missing cl_khr_gl_depth_images define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_depth_images' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_depth_images : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
 #ifndef cl_khr_gl_msaa_sharing
 #error "Missing cl_khr_gl_msaa_sharing 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.

LGTM! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2341638 , @mantognini wrote:

> I don't want to stop the wider discussion, that being said I think I've 
> addressed the comment regarding the content of this PR. Let me know if the 
> latest version is fine or needs further addressing. Thanks.

Thank you for providing comprehensive information regarding the extensions 
being removed. Since there are no kernel language changes in those I still see 
no reason to keep them in clang!

However, regarding the discussion about the general extension mechanisms for 
the language changes, @jvesely would you be willing to describe your vision 
regarding extensions pragma on this issue 
https://github.com/KhronosGroup/OpenCL-Docs/issues/82? Or otherwise, would you 
be ok if I summarise it there? I think it would be useful to aggregate all the 
information before discussing future directions with Khronos.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.
Herald added a subscriber: dexonsmith.

I don't want to stop the wider discussion, that being said I think I've 
addressed the comment regarding the content of this PR. Let me know if the 
latest version is fine or needs further addressing. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2335627 , @jvesely wrote:

> In D89372#2335027 , @Anastasia wrote:
>
>> In D89372#2334728 , @jvesely wrote:
>>
>>> In D89372#2334225 , @Anastasia 
>>> wrote:
>>>
> 

 Ok, so the pragma is supposed to control the pointer dereferencing which 
 seems like a valid case but however, it is not implemented now. Do we know 
 of any immediate plans from anyone to implement this? If not I would 
 prefer to remove the pragma for now? If someone decided to implement this 
 functionality later fully it is absolutely fine. Pragma will be very easy 
 to add. There is no need for everyone to pay the price for something that 
 can't be used at the moment.
>>>
>>> I though the current behaviour of 'you can use #pragma, but the 
>>> dereferences are always legal' was intentional for backward compatibility.
>>> I don't think there are programs that would set it to 'disabled' and expect 
>>> compilation failure.
>>
>> The initial state of the pragma is disabled, so if disabling the extension 
>> isn't supposed to do anything then I don't know why would anyone ever enable 
>> it?
>>
>>> My concern is that legacy apps would set '#pragma enabled' before using 
>>> char/short. such behavior would produce warning/error if with this change 
>>> applied.
>>
>> Correct, it will compile with a warning but not fail to compile. I am 
>> willing to introduce a -W option (if not present already ) to suppress that 
>> warning if it helps with the clean up and backward compatibility. It might 
>> also open up opportunities for a wider clean up  - removing pragma in 
>> extensions that currently require pragma for no good reason.  I have written 
>> more details on this in 
>> https://github.com/KhronosGroup/OpenCL-Docs/pull/355#issuecomment-662679499
>
> Adding a new option cannot be a solution as legacy applications were written 
> before its introduction.
> CL1.x applications need to continue to work without changes, anything else 
> breaks backward compatibility.

Yes, we don't break backward compatibility of the specified behavior. This is 
never the intent.

> The same arguments also apply to `cles_khr_in64`. It's illegal to use 
> int64 types in embedded profile unless you first enable the extensions. 
> Rather than removing it, `cles_khr_2d_image_array_writes` should be added 
> to the extension list.

 I don't think clang currently support embedded profile. Adding extension 
 into the OpenCLOptions is pointless if it's not used. Do you plan to add 
 any functionality for it? Defining macros can be done easily outside of 
 clang source code or if it is preferable to do inside there is 
 `MacroBuilder::defineMacro`  available in the target hooks. Currently for 
 every extension added into `OpenCLExtensions.def` there is a macro, a 
 pragma and a field in `OpenCLOptions` added. This is often more than what 
 is necessary. Plus Khronos has very many extensions if we assume that all 
 of them are added in clang it will become scalability and maintanance 
 nightmare. Most of the extensions only add functions so if we provide a 
 way to add macros for those outside of clang code base it will keep the 
 common code clean and vendors can be more flexible in adding the 
 extensions without the need to modify upstream code if they need to. I see 
 it as an opportunity to improve common code and out of tree 
 implementations too. It just need a little bit of restructuring.
>>>
>>> My understanding is that both the macro and working #pragma directive is 
>>> necessary.
>>
>> I don't believe this is the correct interpretation. If you look at the 
>> extension spec s9.1 it says:
>>
>> `Every extension which affects the OpenCL language semantics, syntax or adds 
>> built-in functions to the language must create a preprocessor #define that 
>> matches the extension name string.  This #define would be available in the 
>> language if and only if the extension is supported on a given 
>> implementation.`
>
> This is only one part indicating the availability of extension. It's not 
> describing what's necessary to observe extended behaviour. The specs state 
> that the initial state of the compiler is as if `#pragma OPENCL EXTENSION all 
> : disable` was issued.
> This means that functions introduced by extensions cannot be present, and the 
> symbol names are available for application use unless such symbols were 
> previously reserved.

This is not written in  the spec. The statement you quote only says that there 
is implicit `#pragma OPENCL EXTENSION all : disable` and this is all. It says 
nothing about functions!

>> It does not say that the pragma is needed, it only says that macro is 
>> needed. That perfectly makes sense because 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

In D89372#2335027 , @Anastasia wrote:

> In D89372#2334728 , @jvesely wrote:
>
>> In D89372#2334225 , @Anastasia 
>> wrote:
>>
 
>>>
>>> Ok, so the pragma is supposed to control the pointer dereferencing which 
>>> seems like a valid case but however, it is not implemented now. Do we know 
>>> of any immediate plans from anyone to implement this? If not I would prefer 
>>> to remove the pragma for now? If someone decided to implement this 
>>> functionality later fully it is absolutely fine. Pragma will be very easy 
>>> to add. There is no need for everyone to pay the price for something that 
>>> can't be used at the moment.
>>
>> I though the current behaviour of 'you can use #pragma, but the dereferences 
>> are always legal' was intentional for backward compatibility.
>> I don't think there are programs that would set it to 'disabled' and expect 
>> compilation failure.
>
> The initial state of the pragma is disabled, so if disabling the extension 
> isn't supposed to do anything then I don't know why would anyone ever enable 
> it?
>
>> My concern is that legacy apps would set '#pragma enabled' before using 
>> char/short. such behavior would produce warning/error if with this change 
>> applied.
>
> Correct, it will compile with a warning but not fail to compile. I am willing 
> to introduce a -W option (if not present already ) to suppress that warning 
> if it helps with the clean up and backward compatibility. It might also open 
> up opportunities for a wider clean up  - removing pragma in extensions that 
> currently require pragma for no good reason.  I have written more details on 
> this in 
> https://github.com/KhronosGroup/OpenCL-Docs/pull/355#issuecomment-662679499

Adding a new option cannot be a solution as legacy applications were written 
before its introduction.
CL1.x applications need to continue to work without changes, anything else 
breaks backward compatibility.

 The same arguments also apply to `cles_khr_in64`. It's illegal to use 
 int64 types in embedded profile unless you first enable the extensions. 
 Rather than removing it, `cles_khr_2d_image_array_writes` should be added 
 to the extension list.
>>>
>>> I don't think clang currently support embedded profile. Adding extension 
>>> into the OpenCLOptions is pointless if it's not used. Do you plan to add 
>>> any functionality for it? Defining macros can be done easily outside of 
>>> clang source code or if it is preferable to do inside there is 
>>> `MacroBuilder::defineMacro`  available in the target hooks. Currently for 
>>> every extension added into `OpenCLExtensions.def` there is a macro, a 
>>> pragma and a field in `OpenCLOptions` added. This is often more than what 
>>> is necessary. Plus Khronos has very many extensions if we assume that all 
>>> of them are added in clang it will become scalability and maintanance 
>>> nightmare. Most of the extensions only add functions so if we provide a way 
>>> to add macros for those outside of clang code base it will keep the common 
>>> code clean and vendors can be more flexible in adding the extensions 
>>> without the need to modify upstream code if they need to. I see it as an 
>>> opportunity to improve common code and out of tree implementations too. It 
>>> just need a little bit of restructuring.
>>
>> My understanding is that both the macro and working #pragma directive is 
>> necessary.
>
> I don't believe this is the correct interpretation. If you look at the 
> extension spec s9.1 it says:
>
> `Every extension which affects the OpenCL language semantics, syntax or adds 
> built-in functions to the language must create a preprocessor #define that 
> matches the extension name string.  This #define would be available in the 
> language if and only if the extension is supported on a given implementation.`

This is only one part indicating the availability of extension. It's not 
describing what's necessary to observe extended behaviour. The specs state that 
the initial state of the compiler is as if `#pragma OPENCL EXTENSION all : 
disable` was issued.
This means that functions introduced by extensions cannot be present, and the 
symbol names are available for application use unless such symbols were 
previously reserved.

> It does not say that the pragma is needed, it only says that macro is needed. 
> That perfectly makes sense because the macro allows to check that the 
> extension is present to implement certain functionality conditionally.

This cited line only talks about extensions availability, not extension use. 
One example of newly introduced functions is in cl_khr_in64_base_atomics. The 
specs say:

  The optional extensions cl_khr_int64_base_atomics and 
cl_khr_int64_extended_atomics
  implement atomic operations on 64-bit signed and unsigned integers to 
locations in __global
  

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 298660.
mantognini added a comment.

Keep cl_khr_byte_addressable_store and cles_khr_int64 out of this PR. Add more 
details on the extensions being removed in the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,16 +34,6 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
 #ifndef cl_khr_byte_addressable_store
@@ -86,15 +76,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,24 +94,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
 #ifndef cles_khr_int64
 #error "Missing cles_khr_int64 define"
@@ -140,60 +103,6 @@
 #endif
 #pragma OPENCL EXTENSION cles_khr_int64 : enable
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_image2d_from_buffer : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_initialize_memory
-#error "Missing cl_khr_initialize_memory define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_initialize_memory' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_initialize_memory : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_gl_depth_images
-#error "Missing cl_khr_gl_depth_images define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_depth_images' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_depth_images : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
 #ifndef cl_khr_gl_msaa_sharing
 #error "Missing cl_khr_gl_msaa_sharing define"
@@ -203,33 +112,6 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2334728 , @jvesely wrote:

> In D89372#2334225 , @Anastasia wrote:
>
>>> 
>>
>> Ok, so the pragma is supposed to control the pointer dereferencing which 
>> seems like a valid case but however, it is not implemented now. Do we know 
>> of any immediate plans from anyone to implement this? If not I would prefer 
>> to remove the pragma for now? If someone decided to implement this 
>> functionality later fully it is absolutely fine. Pragma will be very easy to 
>> add. There is no need for everyone to pay the price for something that can't 
>> be used at the moment.
>
> I though the current behaviour of 'you can use #pragma, but the dereferences 
> are always legal' was intentional for backward compatibility.
> I don't think there are programs that would set it to 'disabled' and expect 
> compilation failure.

The initial state of the pragma is disabled, so if disabling the extension 
isn't supposed to do anything then I don't know why would anyone ever enable it?

> My concern is that legacy apps would set '#pragma enabled' before using 
> char/short. such behavior would produce warning/error if with this change 
> applied.

Correct, it will compile with a warning but not fail to compile. I am willing 
to introduce a -W option (if not present already ) to suppress that warning if 
it helps with the clean up and backward compatibility. It might also open up 
opportunities for a wider clean up  - removing pragma in extensions that 
currently require pragma for no good reason.  I have written more details on 
this in 
https://github.com/KhronosGroup/OpenCL-Docs/pull/355#issuecomment-662679499

>>> The same arguments also apply to `cles_khr_in64`. It's illegal to use int64 
>>> types in embedded profile unless you first enable the extensions. Rather 
>>> than removing it, `cles_khr_2d_image_array_writes` should be added to the 
>>> extension list.
>>
>> I don't think clang currently support embedded profile. Adding extension 
>> into the OpenCLOptions is pointless if it's not used. Do you plan to add any 
>> functionality for it? Defining macros can be done easily outside of clang 
>> source code or if it is preferable to do inside there is 
>> `MacroBuilder::defineMacro`  available in the target hooks. Currently for 
>> every extension added into `OpenCLExtensions.def` there is a macro, a pragma 
>> and a field in `OpenCLOptions` added. This is often more than what is 
>> necessary. Plus Khronos has very many extensions if we assume that all of 
>> them are added in clang it will become scalability and maintanance 
>> nightmare. Most of the extensions only add functions so if we provide a way 
>> to add macros for those outside of clang code base it will keep the common 
>> code clean and vendors can be more flexible in adding the extensions without 
>> the need to modify upstream code if they need to. I see it as an opportunity 
>> to improve common code and out of tree implementations too. It just need a 
>> little bit of restructuring.
>
> My understanding is that both the macro and working #pragma directive is 
> necessary.

I don't believe this is the correct interpretation. If you look at the 
extension spec s9.1 it says:

`Every extension which affects the OpenCL language semantics, syntax or adds 
built-in functions to the language must create a preprocessor #define that 
matches the extension name string.  This #define would be available in the 
language if and only if the extension is supported on a given implementation.`

It does not say that the pragma is needed, it only says that macro is needed. 
That perfectly makes sense because the macro allows to check that the extension 
is present to implement certain functionality conditionally.

OpenCL spec however never clarified the use of pragma that technically makes 
sense because the pragmas in C languages are used for altering the standard 
behavior that can't be otherwise achieved using standard parsing i.e. C99 
section 6.10.1 says about non-STDC pragmas:

`The behavior might cause translation to fail or cause the translator  or  the  
resulting  program  to  behave  in  a  non-conforming  manner.   Any  such 
pragma that is not recognized by the implementation is ignored.`

So C99 only regulates behavior of STDC pragmas and for those it explicitly says 
how the behavior of the parsed program is altered.

Technically OpenCL pragmas are not covered neither in OpenCL C and not C99 and 
therefore it is unclear what the implementation should do. However, with time 
due to the absence of the clarification mutiple interpretations appeared. Sadly 
some of them ended up in a very suboptimal state both for the tooling and the 
application developers because the pragma started to be added for no reason or 
for controlling redundant diagnostics of use of types or functions that 
extensions were introducing. If you are interested in more 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

In D89372#2334225 , @Anastasia wrote:

>> 
>
> Ok, so the pragma is supposed to control the pointer dereferencing which 
> seems like a valid case but however, it is not implemented now. Do we know of 
> any immediate plans from anyone to implement this? If not I would prefer to 
> remove the pragma for now? If someone decided to implement this functionality 
> later fully it is absolutely fine. Pragma will be very easy to add. There is 
> no need for everyone to pay the price for something that can't be used at the 
> moment.

I though the current behaviour of 'you can use #pragma, but the dereferences 
are always legal' was intentional for backward compatibility.
I don't think there are programs that would set it to 'disabled' and expect 
compilation failure. My concern is that legacy apps would set '#pragma enabled' 
before using char/short. such behavior would produce warning/error if with this 
change applied.

>> The same arguments also apply to `cles_khr_in64`. It's illegal to use int64 
>> types in embedded profile unless you first enable the extensions. Rather 
>> than removing it, `cles_khr_2d_image_array_writes` should be added to the 
>> extension list.
>
> I don't think clang currently support embedded profile. Adding extension into 
> the OpenCLOptions is pointless if it's not used. Do you plan to add any 
> functionality for it? Defining macros can be done easily outside of clang 
> source code or if it is preferable to do inside there is 
> `MacroBuilder::defineMacro`  available in the target hooks. Currently for 
> every extension added into `OpenCLExtensions.def` there is a macro, a pragma 
> and a field in `OpenCLOptions` added. This is often more than what is 
> necessary. Plus Khronos has very many extensions if we assume that all of 
> them are added in clang it will become scalability and maintanance nightmare. 
> Most of the extensions only add functions so if we provide a way to add 
> macros for those outside of clang code base it will keep the common code 
> clean and vendors can be more flexible in adding the extensions without the 
> need to modify upstream code if they need to. I see it as an opportunity to 
> improve common code and out of tree implementations too. It just need a 
> little bit of restructuring.

My understanding is that both the macro and working #pragma directive is 
necessary. The configuration bit is only needed if clang changes behaviour, 
which is a separate question.
I'd also argue that new third party extensions need an API call to register new 
extensions in order to get a working pragma mechanism.

Even if an extension only adds access new functions, pragma should control if 
user functions with conflicting names are legal or not.

for example, a program can implement function `new_func`, which gets later 
added to an extension. since the program doesn't know about the new extension 
it doesn't use `#pragma new_extension:enable` and continues to work correctly.
If the new extension exposes `new_func` unconditionally, the program breaks, 
because it doesn't check for a macro that didn't exist at the time it was 
written.
more recent programs can use ifdef to either use `new_func` provided by the 
extension, or implement a custom version.

I didn't find much about embedded program behavior in full profile 
implementation in the specs.
It only says that "embedded profile is a subset" which imo implies that legal 
embedded profile programs should work correctly in a full profile 
implementation.
This implies that cles_* pragmas should continue to work even if the behavior 
is always supported.

>>> Are you suggesting to leave this out? However I don't see any evidence that 
>>> this require either macro or pragma. I feel this is in rather incomplete 
>>> state. So I don't feel we can accomodate for all of these.
>>
>> "incomplete specification" is imo a good reason to drop support for an 
>> extension. My argument is that explanation of legacy extensions should rely 
>> on the spec that introduced them, rather than the current 2.x/3.x which does 
>> an arguably poor job on backward compatibility.
>
> Ok, the idea is not to break backwards compatibility of course. For the 
> extensions that intended to modify language (but never did) if we look from 
> upstream clang user's perspective those extensions couldn't possibly be used 
> to do anything useful. It is of course possible that in some forks the 
> functionality has been completed but then they can easily update the 
> implementation to include the extension definition back. This is very small 
> change compared to the actual extension functionality.
>
> I am ok to leave the extensions that could hypotetically modify the language 
> out of this patch for now. Perhaps we could add a comment explaining that 
> they are unused and only left for backwards compatibility. In a long term we 
> need to find some ways to remove the legacy 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2333937 , @jvesely wrote:

> In D89372#2332997 , @Anastasia wrote:
>
>> In D89372#2332853 , @jvesely wrote:
>>
>>> `cl_khr_byte_addressable_stores` changes language semantics. without it, 
>>> pointer dereferences of types smaller than 32 bits are illegal.
>>
>> Ok, does it mean that we are missing to diagnose this in the frontend? Btw I 
>> do acknowledge that what you say makes sense but I don't think the 
>> documentation support that:
>> https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_byte_addressable_store.html
>>
>> Am I just looking in the wrong place?
>
> Since it's only an extension in ocl 1.0 that spec is a better place to look:
>
>   9.9 Byte Addressable Stores 
>   Section 6.8.m describes restrictions on built-in types char, uchar, char2, 
> uchar2, short,
>   and half. The OpenCL extension cl_khr_byte_addressable_store removes these
>   restrictions.  An application that wants to be able to write to elements of 
> a pointer (or struct) that
>   are of type char, uchar, char2, uchar2, short, ushort and half will need to 
> include
>   the #pragma OPENCL EXTENSION cl_khr_byte_addressable_store :
>   enable directive before any code that performs writes that may not be 
> supported as per section
>   6.8.m.
>   
>   6.8 Restrictions
>   ...
>   m. Built-in types that are less than 32-bits in size i.e. char, uchar, 
> char2, uchar2,
>   short, ushort, and half have the following restriction:
>   Writes to a pointer (or arrays) of type char, uchar, char2, uchar2, short,
>   ushort, and half or to elements of a struct that are of type char, uchar,
>   char2, uchar2, short and ushort are not supported. Refer to section 9.9
>   for additional information.

Ok, thanks! I guess the documentation should be fixed in this URL to point to 
the place where this behavior is described, like for example in 
cl_khr_subgroups:
https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_subgroups.html

>>> Even if all clang targets support this the macro should still be defined 
>>> for backward compatibility.
>>
>> Ok, are you saying that all targets currently support this and we sould 
>> always define it? In this case I would be more happy if we move them into 
>> the internal header that we add implicitly anyway...
>
> r600/r700/EG/NI targets have a bit wonky support byte-addressable stores 
> (basically using 32bit atomic MSKOR), but I don't expect those targets to 
> survive for long, and for now, they advertise support.
> some out of tree backends might also benefit from the extension (like legup 
> -- verilog backend), but I'm not sure if out of tree targets are worth 
> considering, or if they indeed make use of this extension.
> on a high-level, I could see a restriction to 32bit data paths be useful for 
> FPGA targets.
>
> moving the define it to header breaks OCL1.0 programs. Based on the specs 
> above. those programs are required to include the following:
>
>   #ifdef cl_khr_byte_addressable_store
>   #pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
>   #endif
>
> Before they can dereference pointers to char/char2/short/... types (including 
> array indexing and struct members).
> so the `pragma` part needs to work, and the language level checks need to 
> work if the extension is not enabled.

Ok, so the pragma is supposed to control the pointer dereferencing which seems 
like a valid case but however, it is not implemented now. Do we know of any 
immediate plans from anyone to implement this? If not I would prefer to remove 
the pragma for now? If someone decided to implement this functionality later 
fully it is absolutely fine. Pragma will be very easy to add. There is no need 
for everyone to pay the price for something that can't be used at the moment.

> The same arguments also apply to `cles_khr_in64`. It's illegal to use int64 
> types in embedded profile unless you first enable the extensions. Rather than 
> removing it, `cles_khr_2d_image_array_writes` should be added to the 
> extension list.

I don't think clang currently support embedded profile. Adding extension into 
the OpenCLOptions is pointless if it's not used. Do you plan to add any 
functionality for it? Defining macros can be done easily outside of clang 
source code or if it is preferable to do inside there is 
`MacroBuilder::defineMacro`  available in the target hooks. Currently for every 
extension added into `OpenCLExtensions.def` there is a macro, a pragma and a 
field in `OpenCLOptions` added. This is often more than what is necessary. Plus 
Khronos has very many extensions if we assume that all of them are added in 
clang it will become scalability and maintanance nightmare. Most of the 
extensions only add functions so if we provide a way to add macros for those 
outside of clang code base it will keep the common 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

In D89372#2332997 , @Anastasia wrote:

> In D89372#2332853 , @jvesely wrote:
>
>> `cl_khr_byte_addressable_stores` changes language semantics. without it, 
>> pointer dereferences of types smaller than 32 bits are illegal.
>
> Ok, does it mean that we are missing to diagnose this in the frontend? Btw I 
> do acknowledge that what you say makes sense but I don't think the 
> documentation support that:
> https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_byte_addressable_store.html
>
> Am I just looking in the wrong place?

Since it's only an extension in ocl 1.0 that spec is a better place to look:

  9.9 Byte Addressable Stores 
  Section 6.8.m describes restrictions on built-in types char, uchar, char2, 
uchar2, short,
  and half. The OpenCL extension cl_khr_byte_addressable_store removes these
  restrictions.  An application that wants to be able to write to elements of a 
pointer (or struct) that
  are of type char, uchar, char2, uchar2, short, ushort and half will need to 
include
  the #pragma OPENCL EXTENSION cl_khr_byte_addressable_store :
  enable directive before any code that performs writes that may not be 
supported as per section
  6.8.m.
  
  6.8 Restrictions
  ...
  m. Built-in types that are less than 32-bits in size i.e. char, uchar, char2, 
uchar2,
  short, ushort, and half have the following restriction:
  Writes to a pointer (or arrays) of type char, uchar, char2, uchar2, short,
  ushort, and half or to elements of a struct that are of type char, uchar,
  char2, uchar2, short and ushort are not supported. Refer to section 9.9
  for additional information.



>> Even if all clang targets support this the macro should still be defined for 
>> backward compatibility.
>
> Ok, are you saying that all targets currently support this and we sould 
> always define it? In this case I would be more happy if we move them into the 
> internal header that we add implicitly anyway...

r600/r700/EG/NI targets have a bit wonky support byte-addressable stores 
(basically using 32bit atomic MSKOR), but I don't expect those targets to 
survive for long, and for now, they advertise support.
some out of tree backends might also benefit from the extension (like legup -- 
verilog backend), but I'm not sure if out of tree targets are worth 
considering, or if they indeed make use of this extension.
on a high-level, I could see a restriction to 32bit data paths be useful for 
FPGA targets.

moving the define it to header breaks OCL1.0 programs. Based on the specs 
above. those programs are required to include the following:

  #ifdef cl_khr_byte_addressable_store
  #pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
  #endif

Before they can dereference pointers to char/char2/short/... types (including 
array indexing and struct members).
so the `pragma` part needs to work, and the language level checks need to work 
if the extension is not enabled.

The same arguments also apply to `cles_khr_in64`. It's illegal to use int64 
types in embedded profile unless you first enable the extensions. Rather than 
removing it, `cles_khr_2d_image_array_writes` should be added to the extension 
list.

clang can always decide that OCL1.0 programs (even when compiled in cl-1.x 
mode) and embedded profile is unsupported.
I have no clue if there are many OCL programs that rely on those modes out 
there.
However, removing support for them is a significantly bigger change than 
cleaning up a few language-irrelevant extensions (actually, I'm not sure if 
clang ever supported OCL embedded mode).

>> it would be useful if the commit message or the description of this revision 
>> included a justification for each removed extension why it doesn't impact 
>> language semantics with spec references.
>
> Yes, this is a good suggestion in principle and we should try our best. 
> However I am not sure it is feasible for all of those, in particular this 
> documentation doesn't contain anything:
> https://man.opencl.org/cl_khr_context_abort.html
>
> Are you suggesting to leave this out? However I don't see any evidence that 
> this require either macro or pragma. I feel this is in rather incomplete 
> state. So I don't feel we can accomodate for all of these.

"incomplete specification" is imo a good reason to drop support for an 
extension. My argument is that explanation of legacy extensions should rely on 
the spec that introduced them, rather than the current 2.x/3.x which does an 
arguably poor job on backward compatibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2332853 , @jvesely wrote:

> `cl_khr_byte_addressable_stores` changes language semantics. without it, 
> pointer dereferences of types smaller than 32 bits are illegal.

Ok, does it mean that we are missing to diagnose this in the frontend? Btw I do 
acknowledge that what you say makes sense but I don't think the documentation 
support that:
https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_byte_addressable_store.html

Am I just looking in the wrong place?

> Even if all clang targets support this the macro should still be defined for 
> backward compatibility.

Ok, are you saying that all targets currently support this and we sould always 
define it? In this case I would be more happy if we move them into the internal 
header that we add implicitly anyway...

> it would be useful if the commit message or the description of this revision 
> included a justification for each removed extension why it doesn't impact 
> language semantics with spec references.

Yes, this is a good suggestion in principle and we should try our best. However 
I am not sure it is feasible for all of those, in particular this documentation 
doesn't contain anything:
https://man.opencl.org/cl_khr_context_abort.html

Are you suggesting to leave this out? However I don't see any evidence that 
this require either macro or pragma. I feel this is in rather incomplete state. 
So I don't feel we can accomodate for all of these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

`cl_khr_byte_addressable_stores` changes language semantics. without it, 
pointer dereferences of types smaller than 32 bits are illegal.
Even if all clang targets support this the macro should still be defined for 
backward compatibility.

it would be useful if the commit message or the description of this revision 
included a justification for each removed extension why it doesn't impact 
language semantics with spec references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:16
 //
 // If the extensions are to be enumerated without the supported OpenCL version,
 // define OPENCLEXT(ext) where ext is the name of the extension.

Anastasia wrote:
> yaxunl wrote:
> > Can you add a comment here referring the spec about "Every extension which 
> > affects the OpenCL language semantics, syntax or adds built-in functions to 
> > the language must create a preprocessor #define that matches the extension 
> > name string." and therefore only extensions "which affects the OpenCL 
> > language semantics, syntax or adds built-in functions to the language" are 
> > defined in this file. Thanks.
> Good idea! I also suggest we add clarifications regarding pragmas, that those 
> are to be added only when the compiler needs to change the compilation flow 
> i.e. if there is a difference in the language semantic from what is defined 
> in a standard.
I've added a comment. Let me know if it suits you.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

Anastasia wrote:
> azabaznov wrote:
> > mantognini wrote:
> > > azabaznov wrote:
> > > > cl_khr_srgb_image_writes - Extension allowing writes to sRGB images 
> > > > from a kernel. This extension enables write_imagef built-in function as 
> > > > it described [[ 
> > > > https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
> > > >  | here]]. So I think we shouldn't remove it. Do I miss something?
> > > On second reading, this one is slightly ambiguous. On the language side, 
> > > the extension doesn't add an overload; it only specifies that existing 
> > > overload can be used with a different kind of image. On the API side, it 
> > > does extend the set of features. But at the same time if the extended API 
> > > is not supported, it's not possible to create an image in the first place 
> > > and therefore impossible to call write_imagef. So I question the 
> > > usefulness of such macro on the device side. Does this argument convince 
> > > you it should be removed?
> > > it's not possible to create an image in the first place and therefore 
> > > impossible
> > Not quite that right. Referring to [[ 
> > https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#conversion-rules-for-srgba-and-sbgra-images
> >  | this ]]:
> > 
> > read_imagef built-in functions for OpenCL C 2.0 devices do implicit 
> > conversions to RGB for sRGB images. However, implicit conversion from sRGB 
> > to RGB is done on write_imagef only if corresponding extension is 
> > supported. Otherwise, explicit conversions inside a kernel may be required.
> > 
> > I'm agree that this one is kind of confusing and requires some extra 
> > investigation. But I think now we should remove only those extensions which 
> > are only API related for sure.
> Ok, thanks for providing extra information. I agree this deserves extra 
> clarifications.
> 
> I am not sure whether the frontend will be able to perform such conversions 
> since it doesn't have information regarding the channels. The image types are 
> completely opaque. That means that potentially the BIFs can handle the 
> conversion internally. However, I am unclear whether the macro can be useful 
> i.e. whether there is anything that can be done differently at the source 
> level based on its availability in OpenCL kernel code.
> 
> It should be ok to leave this out for now from the clean up unless someone 
> else can help quickly with some more insights.
Thanks for the clarification; this indeed requires further investigations so 
I've removed this part from my patch for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 298333.
mantognini added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/Preprocessor/init.c
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,26 +34,8 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
-#ifndef cl_khr_byte_addressable_store
-#error "Missing cl_khr_byte_addressable_store define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110) && defined TEST_CORE_FEATURES
-// expected-warning@-2{{OpenCL extension 'cl_khr_byte_addressable_store' is core feature or supported optional core feature - ignoring}}
-#endif
-
 #ifndef cl_khr_global_int32_base_atomics
 #error "Missing cl_khr_global_int32_base_atomics define"
 #endif
@@ -86,15 +68,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,87 +86,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cles_khr_int64
-#error "Missing cles_khr_int64 define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cles_khr_int64' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cles_khr_int64 : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_image2d_from_buffer : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_initialize_memory
-#error "Missing cl_khr_initialize_memory define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_initialize_memory' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_initialize_memory : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef 

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:16
 //
 // If the extensions are to be enumerated without the supported OpenCL version,
 // define OPENCLEXT(ext) where ext is the name of the extension.

yaxunl wrote:
> Can you add a comment here referring the spec about "Every extension which 
> affects the OpenCL language semantics, syntax or adds built-in functions to 
> the language must create a preprocessor #define that matches the extension 
> name string." and therefore only extensions "which affects the OpenCL 
> language semantics, syntax or adds built-in functions to the language" are 
> defined in this file. Thanks.
Good idea! I also suggest we add clarifications regarding pragmas, that those 
are to be added only when the compiler needs to change the compilation flow 
i.e. if there is a difference in the language semantic from what is defined in 
a standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:16
 //
 // If the extensions are to be enumerated without the supported OpenCL version,
 // define OPENCLEXT(ext) where ext is the name of the extension.

Can you add a comment here referring the spec about "Every extension which 
affects the OpenCL language semantics, syntax or adds built-in functions to the 
language must create a preprocessor #define that matches the extension name 
string." and therefore only extensions "which affects the OpenCL language 
semantics, syntax or adds built-in functions to the language" are defined in 
this file. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D89372#2330868 , @Anastasia wrote:

>> Does the spec requires cl_* macro to be defined if an extension is enabled?
>
> The extension spec currently has:
>
>   Every extension which affects the OpenCL language semantics, syntax or adds 
> built-in functions to the language must create a preprocessor #define that 
> matches the extension name string. This #define would be available in the 
> language if and only if the extension is supported on a given implementation.
>
> Those extensions don't affect the language or add any BIFs so my reading from 
> this the macro shouldn't be available.

Thanks. That sounds reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2330822 , @yaxunl wrote:

> In D89372#2330362 , @Anastasia wrote:
>
>> In D89372#2330217 , @yaxunl wrote:
>>
>>> With this change, clang basically will have no knowledge about the removed 
>>> extensions, i.e., it will not know which extension is supported in which 
>>> version of OpenCL and have no way to enable/disable those extensions. There 
>>> will be no way to define corresponding macros in clang.
>>>
>>> Basically the responsibility of defining those macros will be shifted to 
>>> OpenCL runtime for JIT and shifted to users for offline compilation. They 
>>> need to have knowledge about which extensions are supported in which 
>>> version of OpenCL and which cpu/platform supports them. I am not sure 
>>> whether this is the direction we want to move to.
>>
>> But why do you think anyone would need to use those macros in OpenCL C?
>>
>> Let's take `cl_khr_icd` as an exmaple: 
>> https://www.khronos.org/registry/OpenCL//sdk/2.2/docs/man/html/cl_khr_icd.html
>>
>> `cl_khr_icd - Extension through which the Khronos OpenCL installable client 
>> driver loader (ICD Loader) may expose multiple separate vendor installable 
>> client drivers (Vendor ICDs) for OpenCL.`
>>
>> Why would anyone need any macro while compiling OpenCL C code for this 
>> functionality? It is dialing with the driver loader that runs on the host 
>> before compiling anything.
>>
>> I believe that the addition of such extensions into the kernel language is 
>> accidental and we should try to improve this. There is no need to have 
>> something that isn't needed. We have enough code and complexity to maintain 
>> that is useful. Let's try to simplify by at least removing what is not 
>> needed.
>>
>> On a separate note, the extensions that need macro definition and don't 
>> require the functionality in the clang parsing also doesn't have to be in 
>> the clang source code. I have mentioned it in my RFC as well: 
>> http://lists.llvm.org/pipermail/cfe-dev/2020-September/066911.html
>
> Does the spec requires cl_* macro to be defined if an extension is enabled?

The extension spec currently has:

  Every extension which affects the OpenCL language semantics, syntax or adds 
built-in functions to the language must create a preprocessor #define that 
matches the extension name string. This #define would be available in the 
language if and only if the extension is supported on a given implementation.

Those extensions don't affect the language or add any BIFs so my reading from 
this the macro shouldn't be available.

> If it is not useful, shouldn't the spec be fixed first? Otherwise, how do we 
> make sure users are not using those macros?

I have first attemted to resolve this with Khonos but there was slow progress 
so it got stalled several times 
https://github.com/KhronosGroup/OpenCL-Docs/issues/82 (see also PR). I gather 
we are the main affected stakeholders so perhaps it is up to us to improve this.

As for the spec I would like to point out that clang doesn't implement spec 
presicely from very early standards. The issue is that it has never been 
documented but we plan to improve at least from the documentation side in the 
course of OpenCL 3.0 development.

http://lists.llvm.org/pipermail/cfe-dev/2020-September/066912.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

azabaznov wrote:
> mantognini wrote:
> > azabaznov wrote:
> > > cl_khr_srgb_image_writes - Extension allowing writes to sRGB images from 
> > > a kernel. This extension enables write_imagef built-in function as it 
> > > described [[ 
> > > https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
> > >  | here]]. So I think we shouldn't remove it. Do I miss something?
> > On second reading, this one is slightly ambiguous. On the language side, 
> > the extension doesn't add an overload; it only specifies that existing 
> > overload can be used with a different kind of image. On the API side, it 
> > does extend the set of features. But at the same time if the extended API 
> > is not supported, it's not possible to create an image in the first place 
> > and therefore impossible to call write_imagef. So I question the usefulness 
> > of such macro on the device side. Does this argument convince you it should 
> > be removed?
> > it's not possible to create an image in the first place and therefore 
> > impossible
> Not quite that right. Referring to [[ 
> https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#conversion-rules-for-srgba-and-sbgra-images
>  | this ]]:
> 
> read_imagef built-in functions for OpenCL C 2.0 devices do implicit 
> conversions to RGB for sRGB images. However, implicit conversion from sRGB to 
> RGB is done on write_imagef only if corresponding extension is supported. 
> Otherwise, explicit conversions inside a kernel may be required.
> 
> I'm agree that this one is kind of confusing and requires some extra 
> investigation. But I think now we should remove only those extensions which 
> are only API related for sure.
Ok, thanks for providing extra information. I agree this deserves extra 
clarifications.

I am not sure whether the frontend will be able to perform such conversions 
since it doesn't have information regarding the channels. The image types are 
completely opaque. That means that potentially the BIFs can handle the 
conversion internally. However, I am unclear whether the macro can be useful 
i.e. whether there is anything that can be done differently at the source level 
based on its availability in OpenCL kernel code.

It should be ok to leave this out for now from the clean up unless someone else 
can help quickly with some more insights.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D89372#2330362 , @Anastasia wrote:

> In D89372#2330217 , @yaxunl wrote:
>
>> With this change, clang basically will have no knowledge about the removed 
>> extensions, i.e., it will not know which extension is supported in which 
>> version of OpenCL and have no way to enable/disable those extensions. There 
>> will be no way to define corresponding macros in clang.
>>
>> Basically the responsibility of defining those macros will be shifted to 
>> OpenCL runtime for JIT and shifted to users for offline compilation. They 
>> need to have knowledge about which extensions are supported in which version 
>> of OpenCL and which cpu/platform supports them. I am not sure whether this 
>> is the direction we want to move to.
>
> But why do you think anyone would need to use those macros in OpenCL C?
>
> Let's take `cl_khr_icd` as an exmaple: 
> https://www.khronos.org/registry/OpenCL//sdk/2.2/docs/man/html/cl_khr_icd.html
>
> `cl_khr_icd - Extension through which the Khronos OpenCL installable client 
> driver loader (ICD Loader) may expose multiple separate vendor installable 
> client drivers (Vendor ICDs) for OpenCL.`
>
> Why would anyone need any macro while compiling OpenCL C code for this 
> functionality? It is dialing with the driver loader that runs on the host 
> before compiling anything.
>
> I believe that the addition of such extensions into the kernel language is 
> accidental and we should try to improve this. There is no need to have 
> something that isn't needed. We have enough code and complexity to maintain 
> that is useful. Let's try to simplify by at least removing what is not needed.
>
> On a separate note, the extensions that need macro definition and don't 
> require the functionality in the clang parsing also doesn't have to be in the 
> clang source code. I have mentioned it in my RFC as well: 
> http://lists.llvm.org/pipermail/cfe-dev/2020-September/066911.html

Does the spec requires cl_* macro to be defined if an extension is enabled? If 
it is not useful, shouldn't the spec be fixed first? Otherwise, how do we make 
sure users are not using those macros?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

mantognini wrote:
> azabaznov wrote:
> > cl_khr_srgb_image_writes - Extension allowing writes to sRGB images from a 
> > kernel. This extension enables write_imagef built-in function as it 
> > described [[ 
> > https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
> >  | here]]. So I think we shouldn't remove it. Do I miss something?
> On second reading, this one is slightly ambiguous. On the language side, the 
> extension doesn't add an overload; it only specifies that existing overload 
> can be used with a different kind of image. On the API side, it does extend 
> the set of features. But at the same time if the extended API is not 
> supported, it's not possible to create an image in the first place and 
> therefore impossible to call write_imagef. So I question the usefulness of 
> such macro on the device side. Does this argument convince you it should be 
> removed?
> it's not possible to create an image in the first place and therefore 
> impossible
Not quite that right. Referring to [[ 
https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#conversion-rules-for-srgba-and-sbgra-images
 | this ]]:

read_imagef built-in functions for OpenCL C 2.0 devices do implicit conversions 
to RGB for sRGB images. However, implicit conversion from sRGB to RGB is done 
on write_imagef only if corresponding extension is supported. Otherwise, 
explicit conversions inside a kernel may be required.

I'm agree that this one is kind of confusing and requires some extra 
investigation. But I think now we should remove only those extensions which are 
only API related for sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

azabaznov wrote:
> cl_khr_srgb_image_writes - Extension allowing writes to sRGB images from a 
> kernel. This extension enables write_imagef built-in function as it described 
> [[ 
> https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
>  | here]]. So I think we shouldn't remove it. Do I miss something?
On second reading, this one is slightly ambiguous. On the language side, the 
extension doesn't add an overload; it only specifies that existing overload can 
be used with a different kind of image. On the API side, it does extend the set 
of features. But at the same time if the extended API is not supported, it's 
not possible to create an image in the first place and therefore impossible to 
call write_imagef. So I question the usefulness of such macro on the device 
side. Does this argument convince you it should be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D89372#2330217 , @yaxunl wrote:

> With this change, clang basically will have no knowledge about the removed 
> extensions, i.e., it will not know which extension is supported in which 
> version of OpenCL and have no way to enable/disable those extensions. There 
> will be no way to define corresponding macros in clang.
>
> Basically the responsibility of defining those macros will be shifted to 
> OpenCL runtime for JIT and shifted to users for offline compilation. They 
> need to have knowledge about which extensions are supported in which version 
> of OpenCL and which cpu/platform supports them. I am not sure whether this is 
> the direction we want to move to.

But why do you think anyone would need to use those macros in OpenCL C?

Let's take `cl_khr_icd` as an exmaple: 
https://www.khronos.org/registry/OpenCL//sdk/2.2/docs/man/html/cl_khr_icd.html

`cl_khr_icd - Extension through which the Khronos OpenCL installable client 
driver loader (ICD Loader) may expose multiple separate vendor installable 
client drivers (Vendor ICDs) for OpenCL.`

Why would anyone need any macro while compiling OpenCL C code for this 
functionality. It is dialing with the driver loader that runs on the host 
before compiling anything.

I believe that the addition of such extensions into the kernel language are 
accidental and we should trying to improve this. There is no need to have 
something that isn't needed. We have neough code and complexity to maintain 
that is useful. Let's try to simplify by at lesst removing what is not needed.

On a separate note the extensions that need macro definition and don't require 
the functionality in the clang parsing also doesn't have to be in the clang 
source code. I have mentioned it in my RFC as well: 
http://lists.llvm.org/pipermail/cfe-dev/2020-September/066911.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

With this change, clang basically will have no knowledge about the removed 
extensions, i.e., it will not know which extension is supported in which 
version of OpenCL and have no way to enable/disable those extensions. There 
will be no way to define corresponding macros in clang.

Basically the responsibility of defining those macros will be shifted to OpenCL 
runtime for JIT and shifted to users for offline compilation. They need to have 
knowledge about which extensions are supported in which version of OpenCL and 
which cpu/platform supports them. I am not sure whether this is the direction 
we want to move to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov requested changes to this revision.
azabaznov added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

cl_khr_srgb_image_writes - Extension allowing writes to sRGB images from a 
kernel. This extension enables write_imagef built-in function as it described 
[[ 
https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
 | here]]. So I think we shouldn't remove it. Do I miss something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Yes, this is a group of extensions that doesn't seem to change anything in the 
kernel language. So if the macro is available I don't understand how it can be 
used to do anything different in the kernel code because it just doesn't modify 
anything in the kernel code.

FYI I have attempted to clarify the extensions with Khronos  
https://github.com/KhronosGroup/OpenCL-Docs/issues/82 but it didn't go very far 
unfortunately. I presume this is not important enough and some extensions are 
not even described at all or deprecated. If this is the case they should not 
hold us back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D89372#2329939 , @yaxunl wrote:

> what if users rely on the predefined macros associated with the extension 
> e.g. cl_khr_srgb_image_writes to enable/disable certain code?
>
> What's the issue with these extensions not removed?

I meant to add a link to the RFC that highlighted the issue: 
http://lists.llvm.org/pipermail/cfe-dev/2020-September/066911.html

In a nutshell, those extensions I'm removing are not language extensions but 
api extensions. I can't think of the usefulness of these macros -- if the host 
doesn't support the extensions, the kernels relying on those cannot be executed 
(i.e. it makes no sense). @Anastasia also highlights in her comment that having 
these increases the complexity and maintenance burden in the ecosystem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

what if users rely on the predefined macros associated with the extension e.g. 
cl_khr_srgb_image_writes to enable/disable certain code?

What's the issue with these extensions not removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: cfe-commits, kerbowa, Anastasia, yaxunl, nhaehnle, 
jvesely, jholewinski.
Herald added a project: clang.
mantognini updated this revision to Diff 298093.
mantognini added a comment.
mantognini published this revision for review.

Addressed buildbot issues.


Many non-language extensions are defined but also unused. This patch
removes them with their tests as they do not require compiler support.

The cl_khr_select_fprounding_mode extension is also removed because it
has been deprecated since OpenCL 1.1 and Clang doesn't have any specific
support for it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/Preprocessor/init.c
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,26 +34,8 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
-#ifndef cl_khr_byte_addressable_store
-#error "Missing cl_khr_byte_addressable_store define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110) && defined TEST_CORE_FEATURES
-// expected-warning@-2{{OpenCL extension 'cl_khr_byte_addressable_store' is core feature or supported optional core feature - ignoring}}
-#endif
-
 #ifndef cl_khr_global_int32_base_atomics
 #error "Missing cl_khr_global_int32_base_atomics define"
 #endif
@@ -86,15 +68,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,87 +86,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cles_khr_int64
-#error "Missing cles_khr_int64 define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cles_khr_int64' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cles_khr_int64 : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL