[PATCH] D100984: [OpenCL] Remove the need for subgroupd extension pragma in enqueue kernel builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D100984#2708168 , @azabaznov wrote:

>> Btw I am not suggesting removing the pragma. We will still have to parse it 
>> for backward compatibility. I am only dropping the requirement of using it 
>> in order to call get_kernel_max_sub_group_size_for_ndrange or 
>> get_kernel_sub_group_count_for_ndrange when the extension is supported. 
>> Because I don't see why this would be needed especially that all other 
>> functions from subgroups can be called without the pragma 
>> https://godbolt.org/z/MYavchPT3.
>
> Oh! I didn't get that! I see another one https://godbolt.org/z/3GPW31W9c, 
> https://godbolt.org/z/dYasedxjx. This is also fixed with your patch, right?

True, I have forgotten about those. :)


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

https://reviews.llvm.org/D100984

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

[PATCH] D100984: [OpenCL] Remove the need for subgroupd extension pragma in enqueue kernel builtins

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

> Btw I am not suggesting removing the pragma. We will still have to parse it 
> for backward compatibility. I am only dropping the requirement of using it in 
> order to call get_kernel_max_sub_group_size_for_ndrange or 
> get_kernel_sub_group_count_for_ndrange when the extension is supported. 
> Because I don't see why this would be needed especially that all other 
> functions from subgroups can be called without the pragma 
> https://godbolt.org/z/MYavchPT3.

Oh! I didn't get that! I see another one https://godbolt.org/z/3GPW31W9c, 
https://godbolt.org/z/dYasedxjx. This is also fixed with your patch, right?

> Btw the newer extension specs don't contain the pragma e.g. 
> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#_extended_subgroup_functions

Indeed... No wording about pragma even in OpenCL C 2.0...

> Right now we have the following in the docs: 
> https://clang.llvm.org/docs/OpenCLSupport.html#implementation-guidelines
>
>> Note that some legacy extensions (published prior to OpenCL 3.0) still 
>> provide some non-conformant functionality for pragmas e.g. add diagnostics 
>> on the use of types or functions. This functionality is not guaranteed to 
>> remain in future releases. However, any future changes should not affect 
>> backward compatibility.
>
> This is not quite deprecation but it makes it clear that the behavior can 
> change as soon as backward compatibility is preserved.
>
> Would you like us to add or ammend anything?

I think this fine for this change since the patch doesn't change anything but 
fixes a bug.


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

https://reviews.llvm.org/D100984

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

[PATCH] D100984: [OpenCL] Remove the need for subgroupd extension pragma in enqueue kernel builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D100984#2707980 , @azabaznov wrote:

> I wish it could be true... But `cl_khr_subgroups` still requires pragma in 
> some versions as it wasn't a core feature earlier 
> (https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_subgroups.html).

Btw I am not suggesting removing the pragma. We will still have to parse it for 
backward compatibility. I am only dropping the requirement of using it in order 
to call `get_kernel_max_sub_group_size_for_ndrange` or 
`get_kernel_sub_group_count_for_ndrange` when the extension is supported. 
Because I don't see why this would be needed especially that all other 
functions from subgroups can be called without the pragma 
https://godbolt.org/z/MYavchPT3.

Btw the newer extension specs don't contain the pragma e.g.
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#_extended_subgroup_functions

> I know that we are going around in circles and we discussed this many 
> times... But I see your point and this will definitely lead to overall 
> simplification of OpenCL codebase in clang. Maybe we could add a comment into 
> docs that we are deprecating pragmas?

Right now we have the following in the docs: 
https://clang.llvm.org/docs/OpenCLSupport.html#implementation-guidelines

  Note that some legacy extensions (published prior to OpenCL 3.0) still 
provide some non-conformant functionality for pragmas e.g. add diagnostics on 
the use of types or functions. This functionality is not guaranteed to remain 
in future releases. However, any future changes should not affect backward 
compatibility.

This is not quite deprecation but it makes it clear that the behavior can 
change as soon as backward compatibility is preserved.

Would you like us to add or ammend anything?


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

https://reviews.llvm.org/D100984

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

[PATCH] D100984: [OpenCL] Remove the need for subgroupd extension pragma in enqueue kernel builtins

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added a comment.

I wish it could be true... But `cl_khr_subgroups` still requires pragma in some 
versions as it wasn't a core feature earlier 
(https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_subgroups.html).

I know that we are going around in circles and we discussed this many times... 
But I see your point and this will definitely lead to overall simplification of 
OpenCL codebase in clang. Maybe we could add a comment into docs that we are 
deprecating pragmas?


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

https://reviews.llvm.org/D100984

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

[PATCH] D100984: [OpenCL] Remove the need for subgroupd extension pragma in enqueue kernel builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: svenvh, azabaznov, mantognini.
Herald added subscribers: ebevhan, yaxunl.
Anastasia requested review of this revision.

There is no extension pragma requirement in the spec for these functions and 
all other builtin functions are available without the pragama.

This patch simplifies the parser and makes the language semantics consistent.


https://reviews.llvm.org/D100984

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl


Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir-unknown-unknown" -verify 
-pedantic -fsyntax-only -DB32 -DQUALS=
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir-unknown-unknown" -verify 
-pedantic -fsyntax-only -DB32 -DQUALS= -cl-ext=-cl_khr_subgroups
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir-unknown-unknown" -verify 
-pedantic -fsyntax-only -DB32 -DQUALS="const volatile"
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir64-unknown-unknown" -verify 
-pedantic -fsyntax-only -Wconversion -DWCONV -DQUALS=
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir64-unknown-unknown" -verify 
-pedantic -fsyntax-only -Wconversion -DWCONV -DQUALS="const volatile"
@@ -212,8 +213,8 @@
   size = get_kernel_preferred_work_group_size_multiple(block_A, 1); // 
expected-error{{too many arguments to function call, expected 1, have 2}}
 }
 
+#ifdef cl_khr_subgroups
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
-
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
@@ -231,7 +232,7 @@
 }
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
-
+#else
 kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
@@ -243,3 +244,4 @@
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error 
{{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires 
cl_khr_subgroups support}}
 }
+#endif // ifdef cl_khr_subgroups
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -838,8 +838,7 @@
 }
 
 static bool checkOpenCLSubgroupExt(Sema &S, CallExpr *Call) {
-  if (!S.getOpenCLOptions().isAvailableOption("cl_khr_subgroups",
-  S.getLangOpts())) {
+  if (!S.getOpenCLOptions().isSupported("cl_khr_subgroups", S.getLangOpts())) {
 S.Diag(Call->getBeginLoc(), diag::err_opencl_requires_extension)
 << 1 << Call->getDirectCallee() << "cl_khr_subgroups";
 return true;


Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir-unknown-unknown" -verify -pedantic -fsyntax-only -DB32 -DQUALS=
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir-unknown-unknown" -verify -pedantic -fsyntax-only -DB32 -DQUALS= -cl-ext=-cl_khr_subgroups
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir-unknown-unknown" -verify -pedantic -fsyntax-only -DB32 -DQUALS="const volatile"
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir64-unknown-unknown" -verify -pedantic -fsyntax-only -Wconversion -DWCONV -DQUALS=
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -triple "spir64-unknown-unknown" -verify -pedantic -fsyntax-only -Wconversion -DWCONV -DQUALS="const volatile"
@@ -212,8 +213,8 @@
   size = get_kernel_preferred_work_group_size_multiple(block_A, 1); // expected-error{{too many arguments to function call, expected 1, have 2}}
 }
 
+#ifdef cl_khr_subgroups
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
-
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
@@ -231,7 +232,7 @@
 }
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
-
+#else
 kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
@@ -243,3 +244,4 @@
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires cl_khr_subgroups support}}
 }
+#endif // ifdef cl_khr_subgroups
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -838,8 +838,7 @@
 }
 
 static bool checkOpenCLSubgroupExt(Sema &S, CallExpr *Call) {
-  if (!S.getOpenCLOptions().isAvailableOption("cl_khr_subgroups",
-  S.getLangOpts())) {
+  if (!S.getOpenCLOptions().isSupported("cl_khr_subgroups", S.getLangOpts())) {
 S.Diag(Call->getBeginLoc(), diag::err_opencl_requires_extension)
 << 1