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

Sun Oct 8 11:50:47 PDT 2017

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.

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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/libclc-dev/attachments/20171008/258bad77/attachment.sig>