[Libclc-dev] Add initial support for half precision builtins (take 3)

[Jan Vesely via Libclc-dev]

Hi,

thank you both, for fast feedback. I've attached v2 that should address
most of the comments. I left larger whitespace cleanup for another day.


On Tue, 2018-05-15 at 22:46 +0200, Jeroen Ketema via Libclc-dev wrote:
> Hi Jan,
> 
> > On 15 May 2018, at 05:09, Jan Vesely via Libclc-dev <libclc-dev at lists.llvm.org> wrote:
> > 
> > Hi,
> > 
> > I've tried to post this patch but it's too big for the ML (and I don't
> > know who to bug to get it released).
> > 
> > It adds basic fp16 builtins. The rest will probably need a fp32 wrapper
> > (especially the sw implementations of math functions), but that needs
> > fixed conversion routines.
> 
> Is that going to fly? Wouldn’t you run into double rounding issues?

It probably will, but it's the fastest solution. The alternative is to
adapt all sw routines to half precision (including tables). At least
GCN llvm backend does this for some ops anyway.

Fixing conversion routines is higher priority. We can revisit this
after.

regards,
Jan

> 
> In addition to Aaron’s comments:
> 
> +#define HALF_RADIX 	2
> +#define HALF_MAX 	0x1.ffcp15h
> +#define HALF_MIN 	0x1.0p-14h
> +#define HALF_EPSILON 	0x1.0p-10h
> 
> There seems to be both a space and a tab between the macro name and
> its definition. I also see a few places where indentation is done
> with a tab, e.g., generic/lib/relational/isunordered.cl.
> 
> From nan.inc:
> 
>  #if __CLC_FPSIZE == 64
>  #define __CLC_NATN __CLC_XCONCAT(ulong, __CLC_VECSIZE)
> -#else
> +#elif __CLC_FPSIZE == 32
>  #define __CLC_NATN __CLC_XCONCAT(uint, __CLC_VECSIZE)
> +#else
> +#define __CLC_NATN __CLC_XCONCAT(ushort, __CLC_VECSIZE)
>  #endif
> 
> Wouldn’t it make more sense to also use “#elif __CLC_FPSIZE == 16”, and maybe #error otherwise? Similar in a few other places.
> 
> In generic/lib/math/acos.inc:
> 
> +#else
> +#define __CLC_CONST(x) xh
> 
> I think there should be a ## between the x and h.
> 
> In generic/include/math/clc_ldexp.h, the guarded text in indented, while indentation is not used anywhere else.
> 
> I see some cases where the "+#if __CLC_FPSIZE > 16” guard does not have a TODO attached to it, is that on purpose?
> 
> In generic/lib/math/modf.inc casts 0.0f, while generic/lib/math/fract.inc introduces a ZERO macro. Maybe this should be made more uniform (doesn’t have to be as part of this patch).
> 
> Near the end of the patch the int/vector of short difference is explained in a comment "returns an int, but the vector versions return short”. Maybe add a similar comment to generic/include/clc/relational/floatn.inc?
> 
> Jeroen
> 
> > 
> > This should be enough to make clpeak happy (BZ: 96897)
> > 
> > Jan
> > -- 
> > Jan Vesely <jan.vesely at rutgers.edu><0001-Add-initial-support-for-half-precision-builtins.patch.gz>_______________________________________________
> > Libclc-dev mailing list
> > Libclc-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev
> 
> _______________________________________________
> Libclc-dev mailing list
> Libclc-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-0001-Add-initial-support-for-half-precision-builtins.patch.gz
Type: application/gzip
Size: 7268 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libclc-dev/attachments/20180515/732158aa/attachment.bin>
-------------- 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/20180515/732158aa/attachment.sig>