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

[Jan Vesely via Libclc-dev]

On Wed, 2018-05-16 at 21:20 +0200, Jeroen Ketema via Libclc-dev wrote:
> > On 16 May 2018, at 00:59, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > 
> > 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.
> 
> Yeah, if the aim is just to get something working, then just wrapping
> the fp32 code is fine.
> 
> One further remark: I think generic/lib/math/modf.inc needs to #undef ZERO.

Thanks. I've fixed this locally. Guess since it was the same definition
the compiler didn't complain.

> 
> One question about generic/lib/math/clc_sqrt_impl.inc:
> 
> +#define __CLC_NAN (half)NAN
> 
> I assume the reason for this is that there’s __builtin_nanh(“”), or something
> like that? (The ‘h’ in there is coming from my imagination.)

yes. there's no __builtin_nanh(), this relies on the conversion
preserving NaN.

> 
> Otherwise, I don’t have any further comments.

thanks,
Jan

> 
> Jeroen
> 
> > 
> > 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 <mailto:Libclc-dev at lists.llvm.org>
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev>
> > 
> > -- 
> > Jan Vesely <jan.vesely at rutgers.edu <mailto:jan.vesely at rutgers.edu>><v2-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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/libclc-dev/attachments/20180516/89f971ed/attachment-0001.sig>