[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8e6099291dcb: [OpenCL] Make generic addrspace optional for 
-fdeclare-opencl-builtins (authored by svenvh).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107769

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl

Index: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
@@ -70,12 +70,15 @@
 
 // Enable extensions that are enabled in opencl-c-base.h.
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+#define __opencl_c_generic_address_space 1
 #define cl_khr_subgroup_extended_types 1
 #define cl_khr_subgroup_ballot 1
 #define cl_khr_subgroup_non_uniform_arithmetic 1
 #define cl_khr_subgroup_clustered_reduce 1
 #define __opencl_c_read_write_images 1
 #endif
+
+#define __opencl_c_named_address_space_builtins 1
 #endif
 
 kernel void test_pointers(volatile global void *global_p, global const int4 *a) {
Index: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
@@ -1,5 +1,12 @@
-// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -finclude-default-header %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-GAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header \
+// RUN: -cl-ext=-__opencl_c_generic_address_space,-__opencl_c_pipes,-__opencl_c_device_enqueue %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
 
 // Test that mix is correctly defined.
 // CHECK-LABEL: @test_float
@@ -32,6 +39,15 @@
   size_t lid = get_local_id(0);
 }
 
+// Test that the correct builtin is called depending on the generic address
+// space feature availability.
+// CHECK-LABEL: @test_generic_optionality
+// CHECK-GAS: call spir_func float @_Z5fractfPU3AS4f
+// CHECK-NOGAS: call spir_func float @_Z5fractfPf
+void test_generic_optionality(float a, float *b) {
+  float res = fract(a, b);
+}
+
 // CHECK: attributes [[ATTR_CONST]] =
 // CHECK-SAME: readnone
 // CHECK: attributes [[ATTR_PURE]] =
Index: clang/lib/Sema/OpenCLBuiltins.td
===
--- clang/lib/Sema/OpenCLBuiltins.td
+++ clang/lib/Sema/OpenCLBuiltins.td
@@ -85,6 +85,8 @@
 def FuncExtKhrGlMsaaSharing  : FunctionExtension<"cl_khr_gl_msaa_sharing">;
 def FuncExtKhrGlMsaaSharingReadWrite : FunctionExtension<"cl_khr_gl_msaa_sharing __opencl_c_read_write_images">;
 
+def FuncExtOpenCLCGenericAddressSpace: FunctionExtension<"__opencl_c_generic_address_space">;
+def FuncExtOpenCLCNamedAddressSpaceBuiltins : FunctionExtension<"__opencl_c_named_address_space_builtins">;
 def FuncExtOpenCLCPipes  : FunctionExtension<"__opencl_c_pipes">;
 def FuncExtOpenCLCWGCollectiveFunctions  : FunctionExtension<"__opencl_c_work_group_collective_functions">;
 def FuncExtOpenCLCReadWriteImages: FunctionExtension<"__opencl_c_read_write_images">;
@@ -591,10 +593,10 @@
   }
 }
 
-let MaxVersion = CL20 in {
+let Extension = FuncExtOpenCLCNamedAddressSpaceBuiltins in {
   defm : MathWithPointer<[GlobalAS, LocalAS, PrivateAS]>;
 }
-let MinVersion = CL20 in {
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : MathWithPointer<[GenericAS]>;
 }
 
@@ -840,10 +842,10 @@
   }
 }
 
-let MaxVersion = CL20 in {
+let Extension = FuncExtOpenCLCNamedAddressSpaceBuiltins in {
   defm : VloadVstore<[GlobalAS, LocalAS, PrivateAS], 1>;
 }
-let MinVersion = CL20 in {
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : VloadVstore<[GenericAS], 1>;
 }
 // vload with constant address space is available regardless of version.
@@ -874,10 +876,10 @@
   }
 }
 
-let MaxVersion = CL20 in {
+let Extension = 

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.



Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:73
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+#define __opencl_c_generic_address_space 1
 #define cl_khr_subgroup_extended_types 1

Anastasia wrote:
> btw this is not correct for C++ for OpenCL 2021 but we are not testing this 
> with C++ for OpenCL 2021 which we should.
> 
> However it doesn't belong to this patch, but would you be able to add a FIXME 
> here to indicate the issue?
I'll add C++ for OpenCL 2021 (and CL3.0) testing in a separate commit.

As discussed offline, we are likely to drop headerless testing from here 
soonish, since we're relying more and more on opencl-c-base.h for 
`-fdeclare-opencl-builtins`.  We essentially have to redefine all the feature 
macros in the test too, when they're already in the header that we're 
deliberately excluding.


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for changing this. I have no issues with this patch as it doesn't 
change the behavior for OpenCL 2.0 but I did create an issue for OpenCL-Docs to 
fix this problem somehow: 
https://github.com/KhronosGroup/OpenCL-Docs/issues/753. So in case, there is 
any feedback we can refine this behavior later on.




Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:73
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+#define __opencl_c_generic_address_space 1
 #define cl_khr_subgroup_extended_types 1

btw this is not correct for C++ for OpenCL 2021 but we are not testing this 
with C++ for OpenCL 2021 which we should.

However it doesn't belong to this patch, but would you be able to add a FIXME 
here to indicate the issue?


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 403705.
svenvh edited the summary of this revision.
svenvh added a comment.

Make use of the `__opencl_c_named_address_space_builtins` internal feature 
added by D118158 .  This should avoid 
affecting OpenCL 2.0.


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

https://reviews.llvm.org/D107769

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl

Index: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
@@ -70,12 +70,15 @@
 
 // Enable extensions that are enabled in opencl-c-base.h.
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+#define __opencl_c_generic_address_space 1
 #define cl_khr_subgroup_extended_types 1
 #define cl_khr_subgroup_ballot 1
 #define cl_khr_subgroup_non_uniform_arithmetic 1
 #define cl_khr_subgroup_clustered_reduce 1
 #define __opencl_c_read_write_images 1
 #endif
+
+#define __opencl_c_named_address_space_builtins 1
 #endif
 
 kernel void test_pointers(volatile global void *global_p, global const int4 *a) {
Index: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
@@ -1,5 +1,12 @@
-// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -finclude-default-header %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-GAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header \
+// RUN: -cl-ext=-__opencl_c_generic_address_space,-__opencl_c_pipes,-__opencl_c_device_enqueue %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
 
 // Test that mix is correctly defined.
 // CHECK-LABEL: @test_float
@@ -32,6 +39,15 @@
   size_t lid = get_local_id(0);
 }
 
+// Test that the correct builtin is called depending on the generic address
+// space feature availability.
+// CHECK-LABEL: @test_generic_optionality
+// CHECK-GAS: call spir_func float @_Z5fractfPU3AS4f
+// CHECK-NOGAS: call spir_func float @_Z5fractfPf
+void test_generic_optionality(float a, float *b) {
+  float res = fract(a, b);
+}
+
 // CHECK: attributes [[ATTR_CONST]] =
 // CHECK-SAME: readnone
 // CHECK: attributes [[ATTR_PURE]] =
Index: clang/lib/Sema/OpenCLBuiltins.td
===
--- clang/lib/Sema/OpenCLBuiltins.td
+++ clang/lib/Sema/OpenCLBuiltins.td
@@ -85,6 +85,8 @@
 def FuncExtKhrGlMsaaSharing  : FunctionExtension<"cl_khr_gl_msaa_sharing">;
 def FuncExtKhrGlMsaaSharingReadWrite : FunctionExtension<"cl_khr_gl_msaa_sharing __opencl_c_read_write_images">;
 
+def FuncExtOpenCLCGenericAddressSpace: FunctionExtension<"__opencl_c_generic_address_space">;
+def FuncExtOpenCLCNamedAddressSpaceBuiltins : FunctionExtension<"__opencl_c_named_address_space_builtins">;
 def FuncExtOpenCLCPipes  : FunctionExtension<"__opencl_c_pipes">;
 def FuncExtOpenCLCWGCollectiveFunctions  : FunctionExtension<"__opencl_c_work_group_collective_functions">;
 def FuncExtOpenCLCReadWriteImages: FunctionExtension<"__opencl_c_read_write_images">;
@@ -591,10 +593,10 @@
   }
 }
 
-let MaxVersion = CL20 in {
+let Extension = FuncExtOpenCLCNamedAddressSpaceBuiltins in {
   defm : MathWithPointer<[GlobalAS, LocalAS, PrivateAS]>;
 }
-let MinVersion = CL20 in {
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : MathWithPointer<[GenericAS]>;
 }
 
@@ -840,10 +842,10 @@
   }
 }
 
-let MaxVersion = CL20 in {
+let Extension = FuncExtOpenCLCNamedAddressSpaceBuiltins in {
   defm : VloadVstore<[GlobalAS, LocalAS, PrivateAS], 1>;
 }
-let MinVersion = CL20 in {
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : VloadVstore<[GenericAS], 1>;
 }
 // vload with constant address space is available regardless of version.
@@ -874,10 +876,10 @@
   }
 }
 
-let MaxVersion = CL20 in {
+let Extension = 

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Looping in @yaxunl for further feedback regarding OpenCL 2.0 behavior 
specifically.


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D107769#3265867 , @svenvh wrote:

> In D107769#3265665 , @Anastasia 
> wrote:
>
>> The way I understand the spec for OpenCL C 2.0 is that whenever the address 
>> space of the pointer is not listed it means generic has to be used, here is 
>> one example:
>> https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#vector-data-load-and-store-functions
>>
>>   gentypen vloadn(size_t offset, const gentype *p)
>>   gentypen vloadn(size_t offset, const __constant gentype *p)
>>
>> that has no address space (i.e. `generic`) and `constant` explicitly. So I 
>> think if address spaces are not listed explicitly they are not supposed to 
>> be available.
>
> The unified specification (which "specifies all versions of OpenCL C") seems 
> to be making all overloads available as I understand; it is perhaps subtly 
> different from the previous specification?
>
> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#vector-data-load-and-store-functions
>
>   gentypen vloadn(size_t offset, const __global gentype *p)
>   gentypen vloadn(size_t offset, const __local gentype *p)
>   gentypen vloadn(size_t offset, const __constant gentype *p)
>   gentypen vloadn(size_t offset, const __private gentype *p)
>   
>   For OpenCL C 2.0, or OpenCL C 3.0 or newer with the 
> __opencl_c_generic_address_space feature:
>   
>   gentypen vloadn(size_t offset, const gentype *p)
>
> Since the `__constant` overload should always be available, I think a reader 
> can assume that the overloads directly above and below `__constant` are also 
> always available?  So that the generic overload is an optional addition to 
> the list of overloads.  If not, I'd expect the spec to specify a condition 
> before listing the specific overloads.

I agree that OpenCL 3.0 spec might have broken the backward compatibility for 
OpenCL 2.0, however, I am not convinced it has been done intentionally. 
Perhaps, it's worth to clarify that?


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

In D107769#3265665 , @Anastasia wrote:

> The way I understand the spec for OpenCL C 2.0 is that whenever the address 
> space of the pointer is not listed it means generic has to be used, here is 
> one example:
> https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#vector-data-load-and-store-functions
>
>   gentypen vloadn(size_t offset, const gentype *p)
>   gentypen vloadn(size_t offset, const __constant gentype *p)
>
> that has no address space (i.e. `generic`) and `constant` explicitly. So I 
> think if address spaces are not listed explicitly they are not supposed to be 
> available.

The unified specification (which "specifies all versions of OpenCL C") seems to 
be making all overloads available as I understand; it is perhaps subtly 
different from the previous specification?

https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#vector-data-load-and-store-functions

  gentypen vloadn(size_t offset, const __global gentype *p)
  gentypen vloadn(size_t offset, const __local gentype *p)
  gentypen vloadn(size_t offset, const __constant gentype *p)
  gentypen vloadn(size_t offset, const __private gentype *p)
  
  For OpenCL C 2.0, or OpenCL C 3.0 or newer with the 
__opencl_c_generic_address_space feature:
  
  gentypen vloadn(size_t offset, const gentype *p)

Since the `__constant` overload should always be available, I think a reader 
can assume that the overloads directly above and below `__constant` are also 
always available?  So that the generic overload is an optional addition to the 
list of overloads.  If not, I'd expect the spec to specify a condition before 
listing the specific overloads.

> One implication of adding all address space overloads is that it makes 
> library size larger, but my feeling is that we don't have that many functions 
> with pointers to significantly impace the library size?

This patch should be touching all of them.  Not that many indeed, but it might 
still have a non-negligible impact on OpenCL libraries due to the combination 
of #vector-sizes * #types * #addrspaces.


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.
Herald added a subscriber: Naghasan.

In D107769#3064171 , @svenvh wrote:

> In D107769#2967565 , @Anastasia 
> wrote:
>
>> In D107769#2960441 , @svenvh wrote:
>>
>>> I have done an alternative spin of this patch: instead of introducing 
>>> `RequireDisabledExtension`, simply always make the `private`, `global`, and 
>>> `local` overloads available.  This makes tablegen diverge from `opencl-c.h` 
>>> though.  Perhaps we should also always add make the `private`, `global`, 
>>> and `local` overloads available in `opencl-c.h`?
>>
>> Yes, we could do this indeed as a clang extension. I don't think this will 
>> impact the user code. However, this means vendors will have to add 
>> definitions for extra overloads in OpenCL 2.0 mode?
>
> I wonder if making the `private`, `global`, and `local` overloads always 
> available would even be considered an extension.  Unless I overlooked 
> something, I cannot find anything in the spec saying that the `private`, 
> `global`, and `local` overloads should **not** be available when a `generic` 
> overload is available (even though this is what Clang has always done)?
>
> Making all overloads always available in e.g. CL2.0 mode will indeed affect 
> the generated calls, which means the definitions should be available when 
> consuming the generated IR.

The way I understand the spec for OpenCL C 2.0 is that whenever the address 
space of the pointer is not listed it means generic has to be used, here is one 
example:
https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#vector-data-load-and-store-functions

  gentypen vloadn(size_t offset, const gentype *p)
  gentypen vloadn(size_t offset, const __constant gentype *p)

that has no address space (i.e. `generic`) and `constant` explicitly. So I 
think if address spaces are not listed explicitly they are not supposed to be 
available.

One implication of adding all address space overloads is that it makes library 
size larger, but my feeling is that we don't have that many functions with 
pointers to significantly impace the library size?


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

In D107769#2967565 , @Anastasia wrote:

> In D107769#2960441 , @svenvh wrote:
>
>> I have done an alternative spin of this patch: instead of introducing 
>> `RequireDisabledExtension`, simply always make the `private`, `global`, and 
>> `local` overloads available.  This makes tablegen diverge from `opencl-c.h` 
>> though.  Perhaps we should also always add make the `private`, `global`, and 
>> `local` overloads available in `opencl-c.h`?
>
> Yes, we could do this indeed as a clang extension. I don't think this will 
> impact the user code. However, this means vendors will have to add 
> definitions for extra overloads in OpenCL 2.0 mode?

I wonder if making the `private`, `global`, and `local` overloads always 
available would even be considered an extension.  Unless I overlooked 
something, I cannot find anything in the spec saying that the `private`, 
`global`, and `local` overloads should **not** be available when a `generic` 
overload is available (even though this is what Clang has always done)?

Making all overloads always available in e.g. CL2.0 mode will indeed affect the 
generated calls, which means the definitions should be available when consuming 
the generated IR.


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D107769#2960441 , @svenvh wrote:

> I have done an alternative spin of this patch: instead of introducing 
> `RequireDisabledExtension`, simply always make the `private`, `global`, and 
> `local` overloads available.  This makes tablegen diverge from `opencl-c.h` 
> though.  Perhaps we should also always add make the `private`, `global`, and 
> `local` overloads available in `opencl-c.h`?

Yes, we could do this indeed as a clang extension. I don't think this will 
impact the user code. However, this means vendors will have to add extra 
overloads in OpenCL 2.0 mode?


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

https://reviews.llvm.org/D107769

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

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh updated this revision to Diff 368136.
svenvh edited the summary of this revision.
svenvh added a comment.

I have done an alternative spin of this patch: instead of introducing 
`RequireDisabledExtension`, simply always make the `private`, `global`, and 
`local` overloads available.  This makes tablegen diverge from `opencl-c.h` 
though.  Perhaps we should also always add make the `private`, `global`, and 
`local` overloads available in `opencl-c.h`?


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

https://reviews.llvm.org/D107769

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl


Index: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
@@ -63,6 +63,7 @@
 
 // Enable extensions that are enabled in opencl-c-base.h.
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+#define __opencl_c_generic_address_space 1
 #define cl_khr_subgroup_extended_types 1
 #define cl_khr_subgroup_ballot 1
 #define cl_khr_subgroup_non_uniform_arithmetic 1
Index: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
@@ -1,5 +1,11 @@
-// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown 
-cl-std=CL1.2 -finclude-default-header %s | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown 
-cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header %s | FileCheck 
%s
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown 
-cl-std=CL1.2 -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown 
-cl-std=CL1.2 -fdeclare-opencl-builtins -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown 
-cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-GAS
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown 
-cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header 
-cl-ext=-__opencl_c_generic_address_space,-__opencl_c_pipes %s \
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-NOGAS
 
 // Test that mix is correctly defined.
 // CHECK-LABEL: @test_float
@@ -32,6 +38,15 @@
   size_t lid = get_local_id(0);
 }
 
+// Test that the correct builtin is called depending on the generic address
+// space feature availability.
+// CHECK-LABEL: @test_generic_optionality
+// CHECK-GAS: call spir_func float @_Z5fractfPU3AS4f
+// CHECK-NOGAS: call spir_func float @_Z5fractfPf
+void test_generic_optionality(float a, float *b) {
+  float res = fract(a, b);
+}
+
 // CHECK: attributes [[ATTR_CONST]] =
 // CHECK-SAME: readnone
 // CHECK: attributes [[ATTR_PURE]] =
Index: clang/lib/Sema/OpenCLBuiltins.td
===
--- clang/lib/Sema/OpenCLBuiltins.td
+++ clang/lib/Sema/OpenCLBuiltins.td
@@ -83,6 +83,7 @@
 def FuncExtKhrMipmapImageWrites  : 
FunctionExtension<"cl_khr_mipmap_image_writes">;
 def FuncExtKhrGlMsaaSharing  : 
FunctionExtension<"cl_khr_gl_msaa_sharing">;
 
+def FuncExtOpenCLCGenericAddressSpace: 
FunctionExtension<"__opencl_c_generic_address_space">;
 def FuncExtOpenCLCPipes  : 
FunctionExtension<"__opencl_c_pipes">;
 def FuncExtOpenCLCWGCollectiveFunctions  : 
FunctionExtension<"__opencl_c_work_group_collective_functions">;
 
@@ -566,10 +567,8 @@
   }
 }
 
-let MaxVersion = CL20 in {
-  defm : MathWithPointer<[GlobalAS, LocalAS, PrivateAS]>;
-}
-let MinVersion = CL20 in {
+defm : MathWithPointer<[GlobalAS, LocalAS, PrivateAS]>;
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : MathWithPointer<[GenericAS]>;
 }
 
@@ -824,10 +823,8 @@
   }
 }
 
-let MaxVersion = CL20 in {
-  defm : VloadVstore<[GlobalAS, LocalAS, PrivateAS], 1>;
-}
-let MinVersion = CL20 in {
+defm : VloadVstore<[GlobalAS, LocalAS, PrivateAS], 1>;
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : VloadVstore<[GenericAS], 1>;
 }
 // vload with constant address space is available regardless of version.
@@ -859,10 +856,8 @@
   }
 }
 
-let MaxVersion = CL20 in {
-  defm : VloadVstoreHalf<[GlobalAS, LocalAS, PrivateAS], 1>;
-}
-let MinVersion = CL20 in {
+defm : VloadVstoreHalf<[GlobalAS, LocalAS, PrivateAS], 1>;
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
   defm : VloadVstoreHalf<[GenericAS], 1>;
 }
 // vload with constant address space is available regardless of version.


Index: 

[PATCH] D107769: [OpenCL] Make generic addrspace optional for -fdeclare-opencl-builtins

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh created this revision.
svenvh added reviewers: Anastasia, airlied, azabaznov.
svenvh added a project: clang.
Herald added subscribers: ldrumm, yaxunl.
svenvh requested review of this revision.
Herald added a subscriber: cfe-commits.

Currently, -fdeclare-opencl-builtins always adds the generic address
space overloads of e.g. the vload builtin functions in OpenCL 3.0
mode, even when the generic address space feature is disabled.

Guard the generic address space overloads by the
`__opencl_c_generic_address_space` feature instead of by OpenCL
version.

Add a new field `RequireDisabledExtension` to the `Builtin` class so
that we can make certain builtins available only when an extension is
disabled.  Thus, we can provide generic address space overloads OR
private/global/local address space overloads depending on the generic
address space feature availability.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107769

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
  clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -425,6 +425,8 @@
   const bool IsConv : 1;
   // OpenCL extension(s) required for this overload.
   const unsigned short Extension;
+  // OpenCL extension(s) required to be disabled for this overload.
+  const unsigned short DisabledExtension;
   // OpenCL versions in which this overload is available.
   const unsigned short Versions;
 };
@@ -611,6 +613,8 @@
 
 for (const auto  : SLM.second.Signatures) {
   StringRef ExtName = Overload.first->getValueAsDef("Extension")->getName();
+  StringRef DisabledExtName =
+  Overload.first->getValueAsDef("RequireDisabledExtension")->getName();
   unsigned int MinVersion =
   Overload.first->getValueAsDef("MinVersion")->getValueAsInt("ID");
   unsigned int MaxVersion =
@@ -622,6 +626,7 @@
  << (Overload.first->getValueAsBit("IsConst")) << ", "
  << (Overload.first->getValueAsBit("IsConv")) << ", "
  << FunctionExtensionIndex[ExtName] << ", "
+ << FunctionExtensionIndex[DisabledExtName] << ", "
  << EncodeVersions(MinVersion, MaxVersion) << " },\n";
   Index++;
 }
@@ -648,7 +653,9 @@
 Rec->getValueAsDef("MaxVersion")->getValueAsInt("ID") ==
 Rec2->getValueAsDef("MaxVersion")->getValueAsInt("ID") &&
 Rec->getValueAsDef("Extension")->getName() ==
-Rec2->getValueAsDef("Extension")->getName()) {
+Rec2->getValueAsDef("Extension")->getName() &&
+Rec->getValueAsDef("RequireDisabledExtension")->getName() ==
+Rec2->getValueAsDef("RequireDisabledExtension")->getName()) {
   return true;
 }
   }
@@ -1085,11 +1092,27 @@
 OpenCLBuiltinFileEmitterBase::emitExtensionGuard(const Record *Builtin) {
   StringRef Extensions =
   Builtin->getValueAsDef("Extension")->getValueAsString("ExtName");
-  if (Extensions.empty())
-return "";
+  StringRef DisabledExtensions =
+  Builtin->getValueAsDef("RequireDisabledExtension")
+  ->getValueAsString("ExtName");
+
+  assert((Extensions.empty() || DisabledExtensions.empty()) &&
+ "enabling and disabling extensions simultaneously not supported yet!");
+
+  bool RequireDisabled = false;
+  if (Extensions.empty()) {
+if (DisabledExtensions.empty())
+  return "";
+
+Extensions = DisabledExtensions;
+RequireDisabled = true;
+  }
 
   OS << "#if";
 
+  // At this point, Extensions contains a space-separated list of either
+  // the required extensions or the required-to-be-disabled extensions.
+  // RequireDisabled is true if those extensions need to be disabled.
   SmallVector ExtVec;
   Extensions.split(ExtVec, " ");
   bool isFirst = true;
@@ -1097,7 +1120,11 @@
 if (!isFirst) {
   OS << " &&";
 }
-OS << " defined(" << Ext << ")";
+OS << " ";
+if (RequireDisabled) {
+  OS << "!";
+}
+OS << "defined(" << Ext << ")";
 isFirst = false;
   }
   OS << "\n";
Index: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
@@ -63,6 +63,7 @@
 
 // Enable extensions that are enabled in opencl-c-base.h.
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+#define __opencl_c_generic_address_space 1
 #define cl_khr_subgroup_extended_types 1
 #define cl_khr_subgroup_ballot 1
 #define cl_khr_subgroup_non_uniform_arithmetic 1
Index: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl