[Libclc-dev] [PATCH 3/4] Do not include clc_nextafter header globally

Sun Oct 8 11:53:29 PDT 2017

> On 8 Oct 2017, at 20:50, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> 
> On Sun, 2017-10-08 at 20:30 +0200, Jeroen Ketema wrote:
>>> On 8 Oct 2017, at 20:11, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>>> =20
>>> On Sun, 2017-10-08 at 12:22 +0200, Jeroen Ketema via Libclc-dev wrote:
>>>>> On 8 Oct 2017, at 10:55, Jan Vesely via Libclc-dev =
>> <libclc-dev at lists.llvm.org> wrote:
>>>>> =20
>>>>> Drop unused clc/math/clc_nextafter.h header
>>>>> =20
>>>>> Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
>>>>> ---
>>>>> amdgpu/lib/math/nextafter.cl             |  1 +
>>>>> generic/include/clc/clc.h                |  5 -----
>>>>> generic/include/clc/math/clc_nextafter.h | 11 -----------
>>>>> generic/include/clc/math/gentype.inc     |  2 ++
>>>>> 4 files changed, 3 insertions(+), 16 deletions(-)
>>>>> delete mode 100644 generic/include/clc/math/clc_nextafter.h
>>>>> =20
>>>>> diff --git a/amdgpu/lib/math/nextafter.cl =
>> b/amdgpu/lib/math/nextafter.cl
>>>>> index 6aee0a0..5b4521d 100644
>>>>> --- a/amdgpu/lib/math/nextafter.cl
>>>>> +++ b/amdgpu/lib/math/nextafter.cl
>>>>> @@ -1,5 +1,6 @@
>>>>> #include <clc/clc.h>
>>>>> #include "../lib/clcmacro.h"
>>>>> +#include <math/clc_nextafter.h>
>>>>> =20
>>>>> _CLC_DEFINE_BINARY_BUILTIN(float, nextafter, __clc_nextafter, float, =
>> float)
>>>>> =20
>>>>> diff --git a/generic/include/clc/clc.h b/generic/include/clc/clc.h
>>>>> index adaab90..3701336 100644
>>>>> --- a/generic/include/clc/clc.h
>>>>> +++ b/generic/include/clc/clc.h
>>>>> @@ -264,9 +264,4 @@
>>>>> #include <clc/image/image_defines.h>
>>>>> #include <clc/image/image.h>
>>>>> =20
>>>>> -/* libclc internal defintions */
>>>>> -#ifdef __CLC_INTERNAL
>>>>> -#include <math/clc_nextafter.h>
>>>>> -#endif
>>>>> -
>>>>> #pragma OPENCL EXTENSION all : disable
>>>>> diff --git a/generic/include/clc/math/clc_nextafter.h =
>> b/generic/include/clc/math/clc_nextafter.h
>>>>> deleted file mode 100644
>>>>> index 81c8f36..0000000
>>>>> --- a/generic/include/clc/math/clc_nextafter.h
>>>>> +++ /dev/null
>>>>> @@ -1,11 +0,0 @@
>>>>> -#define __CLC_BODY <clc/math/binary_decl.inc>
>>>>> -
>>>>> -#define __CLC_FUNCTION nextafter
>>>>> -#include <clc/math/gentype.inc>
>>>>> -#undef __CLC_FUNCTION
>>>>> -
>>>>> -#define __CLC_FUNCTION __clc_nextafter
>>>>> -#include <clc/math/gentype.inc>
>>>>> -#undef __CLC_FUNCTION
>>>>> -
>>>>> -#undef __CLC_BODY
>>>>> diff --git a/generic/include/clc/math/gentype.inc =
>> b/generic/include/clc/math/gentype.inc
>>>>> index e6ffad1..954cd00 100644
>>>>> --- a/generic/include/clc/math/gentype.inc
>>>>> +++ b/generic/include/clc/math/gentype.inc
>>>>> @@ -54,6 +54,8 @@
>>>>> =20
>>>>> #ifndef __FLOAT_ONLY
>>>>> #ifdef cl_khr_fp64
>>>>> +#pragma OPENCL EXTENSION cl_khr_fp64 : enable
>>>>> +
>>>>> #define __CLC_SCALAR_GENTYPE double
>>>>> #define __CLC_FPSIZE 64
>>>> =20
>>>> I this this pragma being enabled in the header files in other places. =
>> I=E2=80=99m somewhat
>>>> confused: why is this necessary? Why shouldn=E2=80=99t this only =
>> occur in the source files
>>>> that implement fp64 functionality?
>>> =20
>>> it needs to be enabled every time double type is used. It is true that
>>> clc.h enables it globally (and then disables at the end of the =
>> header).
>>> However, this patch moves math/clc_nextafter.h outside of the main
>>> clc.h header. It thus does not benefit from clc.h global extension
>>> enables.
>>> =20
>>> without this change, clc_nextafter.h would need to add:
>>> #ifdef cl_khr_fp64
>>> #pragma OPENCL EXTENSION cl_khr_fp64 : enable
>>> #endif
>>> =20
>>> I consider moving extension enables to type.inc files preferable so =
>> the
>>> 'user' files don't have to think about it.
>> 
>> I would say that from a strict standards point of view, the pragma =
>> should
>> not occur in the headers at all, and only in the implementation. If a =
>> user
>> wants to use doubles, he/she should add the pragma to his/her own code.
>> However, from a usability point-of-view. I totally understand having the
>> pragma in the headers.
> 
> This does not impact user applications. clc.h includes
> #pragma OPENCL EXTENSION all : disable at the end, so any code needs to
> reenable cl_khr_fp64 if it wants to use double type, even if it
> includes clc.h.
> 
> clang does not distinguish between libclc or other code so the same
> rules about types and extensions apply to us; all .cl files reenable
> cl_khr_fp64 if they implement builtin using double type.

You’re right. I’m being stupid. Sorry for the noise.

Jeroen

> Jan
> 
>> 
>> I=E2=80=99m fine with it either way. I just think it=E2=80=99s good we =
>> realise we=E2=80=99re making an
>> explicit choice here.
>> 
>> Jeroen =20
>> 
>>> =20
>>> Jan
>>> =20
>>>> =20
>>>> Jeroen
>>>> =20
>>>>> =20
>>>>> --=20
>>>>> 2.13.6
>>>>> =20
>>>>> _______________________________________________
>>>>> Libclc-dev mailing list
>>>>> Libclc-dev at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev
>>>> =20
>>>> _______________________________________________
>>>> Libclc-dev mailing list
>>>> Libclc-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev
>>> =20
>>> --=20
>>> Jan Vesely <jan.vesely at rutgers.edu <mailto:jan.vesely at rutgers.edu>>
>> 
>> 
>> 
> [SNIP apple mail noise]
> -- 
> Jan Vesely <jan.vesely at rutgers.edu>