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

[Jeroen Ketema via Libclc-dev]

> 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.

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.)

Otherwise, I don’t have any further comments.

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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libclc-dev/attachments/20180516/60dae226/attachment.html>