Re: [PATCH 09/10] libgcc: Add support for HF mode (aka __fp16) in libbid

[Joseph Myers <joseph@codesourcery.com>]

On Mon, 9 May 2022, Christophe Lyon via Gcc-patches wrote:

> This patch adds support for trunc and extend operations between HF
> mode (__fp16) and Decimal Floating Point formats (_Decimal32,
> _Decimal64 and _Decimal128).
> 
> For simplicity we rely on the implicit conversions inserted by the
> compiler between HF and SD/DF/TF modes.  The existing bid*_to_binary*
> and binary*_to_bid* functions are non-trivial and at this stage it is
> not clear if there is a performance-critical use case involving __fp16
> and _Decimal* formats.

Note that for conversion from DFP to HFmode, double rounding (going via 
SFmode) probably produces incorrectly rounded results in some cases 
(though we already have such incorrect results in the other direction for 
DPD; see bug 97635).

> The patch also adds two executable tests for AArch64, to make sure the
> right functions are used, available (link phase) and functional.

Wouldn't it be better to have tests that apply for all targets supporting 
both HFmode and BID DFP, which includes x86 / x86_64 (with SSE2 support) 
as well, using _Float16 as the type name (and the float16 / 
float16_runtime effective-target keywords to test for the relevant support 
and dg-add-options float16)?

> +HFtype
> +__bid_truncddhf (_Decimal64 x) {
> +  HFtype res;
> +  union decimal64 ux;
> +
> +  ux.d = x;
> +  res = __bid64_to_binary32 (ux.i);
> +  return (res);
> +}

Doesn't this need appropriate LIBGCC2_HAS_HF_MODE || BID_HAS_HF_MODE 
conditionals like some of the other functions you're adding?

> +HFtype
> +__bid_truncsdhf (_Decimal32 x) {
> +  HFtype res;
> +  union decimal32 ux;
> +
> +  ux.d = x;
> +  res = __bid32_to_binary32 (ux.i);
> +  return (res);
> +}

Likewise.

> +HFtype
> +__bid_trunctdhf (_Decimal128 x) {
> +  HFtype res;
> +  union decimal128 ux;
> +
> +  ux.d = x;
> +  res = __bid128_to_binary32 (ux.i);
> +  return (res);
> +}

Likewise.

> +#if LIBGCC2_HAS_HF_MODE
> +typedef float HFtype __attribute__ ((mode (HF)));
> +#endif /* LIBGCC2_HAS_HF_MODE */
>  typedef float SFtype __attribute__ ((mode (SF)));
>  typedef float DFtype __attribute__ ((mode (DF)));
>  #if LIBGCC2_HAS_XF_MODE
> @@ -98,6 +111,12 @@ typedef __attribute__ ((aligned(16))) struct
>  #endif
>  #endif
>  
> +#if BID_HAS_HF_MODE
> +#ifndef HFtype
> +#define HFtype __fp16
> +#endif

That seems wrong; the __fp16 name is specific to Arm / AArch64.  I'd 
expect the typedef alone to be specific; in any case, the "mode" attribute 
is the appropriate way to define this type.

-- 
Joseph S. Myers
joseph@codesourcery.com