This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 3/6] float128: Add strfromf128
On Fri, 26 May 2017 15:56:04 +0000
Joseph Myers <joseph@codesourcery.com> wrote:
> On Fri, 26 May 2017, Gabriel F. T. Gomes wrote:
>
> > diff --git a/NEWS b/NEWS
> > index b4ecd62..8cb17cc 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -66,6 +66,9 @@ Version 2.26
> > * The port to Native Client running on ARMv7-A (--host=arm-nacl) has been
> > removed.
> >
> > +* The function strfromf128, from ISO/IEC TS 18661-3:2015, is added to libc.
> > + It converts a _Float128 value into string.
>
> Such a NEWS entry, for a function not available for all glibc
> configurations, needs to indicate what systems the function is available
> on. (I'd say that means there should be one NEWS entry for all the
> float128 functions, added only when the powerpc64le support is enabled,
> rather than piecemeal entries before then.)
OK.
>
> > diff --git a/include/gmp.h b/include/gmp.h
> > index 95d6c16..e6f635e 100644
> > --- a/include/gmp.h
> > +++ b/include/gmp.h
> > @@ -6,6 +6,8 @@
> >
> > #include <stdlib/gmp.h>
> >
> > +#include <bits/floatn.h>
> > +
> > /* Now define the internal interfaces. */
> > extern mp_size_t __mpn_extract_double (mp_ptr res_ptr, mp_size_t size,
> > int *expt, int *is_neg,
>
> Presumably this belongs in patch 2, which adds a test
> of__HAVE_DISTINCT_FLOAT128 to this header; it seems to have nothing to do
> with this patch.
Indeed.
> > @@ -328,6 +339,52 @@ __printf_fp_l (FILE *fp, locale_t loc,
> > grouping = NULL;
> >
> > /* Fetch the argument value. */
> > +#if __HAVE_DISTINCT_FLOAT128
> > + if (info->is_binary128)
>
> I don't like this duplication of a large section of code.
>
> As I see it, the code inside the conditional only varies between types
> for: the field name (f128 in this case); the type name; the
> __mpn_extract_* function called; the MANT_DIG value. (It *used* to differ
> more, before we moved to using type-generic isnan / signbit / isinf macros
> throughout glibc instead of direct calls to the functions those macros
> might call.)
>
> So refactor the existing code to have a macro for the code inside the
> conditional, used for both the long double and double cases. Then you
> just need to add a third call to the macro for the _Float128 case.
>
> > /* Fetch the argument value. */
> > +#if __HAVE_DISTINCT_FLOAT128
> > + if (info->is_binary128)
>
> The same comment about duplication applies here.
I attached a patch with this refactoring for double and long double. I'll
update this block for float128 in the next version of this patch.
Is the attached patch (with the refactoring) OK for master?
> > diff --git a/stdlib/strfrom-skeleton.c b/stdlib/strfrom-skeleton.c
> > index 811a29c..5841919 100644
> > --- a/stdlib/strfrom-skeleton.c
> > +++ b/stdlib/strfrom-skeleton.c
> > @@ -132,6 +132,12 @@ STRFROM (char *dest, size_t size, const char *format, FLOAT f)
> > which type of floating-point number is being passed. */
> > info.is_long_double = __builtin_types_compatible_p (FLOAT, long double);
> >
> > + /* Similarly, the function strfromf128 passes a floating-point number in
> > + _Float128 format to printf_fp. */
> > +#if __HAVE_DISTINCT_FLOAT128
> > + info.is_binary128 = __builtin_types_compatible_p (FLOAT, _Float128);
> > +#endif
>
> In other places you set is_binary128 even when !__HAVE_DISTINCT_FLOAT128.
> It's not *used* when !__HAVE_DISTINCT_FLOAT128 - is there a deliberate
> choice that in this place it shouldn't be set either?
The reason why I chose to not set it here, is because the struct info is
zero'd by a memset some lines above (the narrowness of the diff context
hid it):
/* Prepare the format specification for printf_fp. */
memset (&info, '\0', sizeof (info));
Would it be more clear to set it anyway?
>From 84ac9620ca9bc76622d69708c279bd1658eebd28 Mon Sep 17 00:00:00 2001
From: "Gabriel F. T. Gomes" <gftg@linux.vnet.ibm.com>
Date: Mon, 29 May 2017 10:49:42 -0300
Subject: [PATCH] Remove duplicated code from __printf_fp_l and __printf_fphex
In __printf_fp_l and __printf_fphex, the blocks of code that are used to read a
double or long double argument, check for special values and convert to
multiprecision are similar. When adding float128 support to libc, more code
would be duplicated to deal with the extra type. This patch moves the
repetitive code to a macro which is now used by double and long double and will
be used for float128 when support is added, thus avoiding more duplication.
Tested for powerpc64le and s390x.
2017-05-29 Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
* stdio-common/printf_fp.c (PRINTF_FP_FETCH): New macro.
(__printf_fp_l): Use the new macro to avoid duplicating code.
* stdio-common/printf_fphex.c (PRINTF_FPHEX_FETCH): New macro.
(__printf_fphex): Use the new macro to avoid duplicating code.
---
stdio-common/printf_fp.c | 130 ++++++++++++++++----------------------------
stdio-common/printf_fphex.c | 110 ++++++++++++++-----------------------
2 files changed, 87 insertions(+), 153 deletions(-)
diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
index 7845d96..6d8c381 100644
--- a/stdio-common/printf_fp.c
+++ b/stdio-common/printf_fp.c
@@ -327,94 +327,58 @@ __printf_fp_l (FILE *fp, locale_t loc,
else
grouping = NULL;
+#define PRINTF_FP_FETCH(FLOAT, VAR, SUFFIX, MANT_DIG) \
+ { \
+ VAR = *(const FLOAT *) args[0]; \
+ \
+ /* Check for special values: not a number or infinity. */ \
+ if (isnan (VAR)) \
+ { \
+ is_neg = signbit (VAR); \
+ if (isupper (info->spec)) \
+ { \
+ special = "NAN"; \
+ wspecial = L"NAN"; \
+ } \
+ else \
+ { \
+ special = "nan"; \
+ wspecial = L"nan"; \
+ } \
+ } \
+ else if (isinf (VAR)) \
+ { \
+ is_neg = signbit (VAR); \
+ if (isupper (info->spec)) \
+ { \
+ special = "INF"; \
+ wspecial = L"INF"; \
+ } \
+ else \
+ { \
+ special = "inf"; \
+ wspecial = L"inf"; \
+ } \
+ } \
+ else \
+ { \
+ p.fracsize = __mpn_extract_##SUFFIX \
+ (fp_input, \
+ (sizeof (fp_input) / sizeof (fp_input[0])), \
+ &p.exponent, &is_neg, VAR); \
+ to_shift = 1 + p.fracsize * BITS_PER_MP_LIMB - MANT_DIG; \
+ } \
+ }
+
/* Fetch the argument value. */
#ifndef __NO_LONG_DOUBLE_MATH
if (info->is_long_double && sizeof (long double) > sizeof (double))
- {
- fpnum.ldbl = *(const long double *) args[0];
-
- /* Check for special values: not a number or infinity. */
- if (isnan (fpnum.ldbl))
- {
- is_neg = signbit (fpnum.ldbl);
- if (isupper (info->spec))
- {
- special = "NAN";
- wspecial = L"NAN";
- }
- else
- {
- special = "nan";
- wspecial = L"nan";
- }
- }
- else if (isinf (fpnum.ldbl))
- {
- is_neg = signbit (fpnum.ldbl);
- if (isupper (info->spec))
- {
- special = "INF";
- wspecial = L"INF";
- }
- else
- {
- special = "inf";
- wspecial = L"inf";
- }
- }
- else
- {
- p.fracsize = __mpn_extract_long_double (fp_input,
- (sizeof (fp_input) /
- sizeof (fp_input[0])),
- &p.exponent, &is_neg,
- fpnum.ldbl);
- to_shift = 1 + p.fracsize * BITS_PER_MP_LIMB - LDBL_MANT_DIG;
- }
- }
+ PRINTF_FP_FETCH (long double, fpnum.ldbl, long_double, LDBL_MANT_DIG)
else
-#endif /* no long double */
- {
- fpnum.dbl = *(const double *) args[0];
+#endif
+ PRINTF_FP_FETCH (double, fpnum.dbl, double, DBL_MANT_DIG)
- /* Check for special values: not a number or infinity. */
- if (isnan (fpnum.dbl))
- {
- is_neg = signbit (fpnum.dbl);
- if (isupper (info->spec))
- {
- special = "NAN";
- wspecial = L"NAN";
- }
- else
- {
- special = "nan";
- wspecial = L"nan";
- }
- }
- else if (isinf (fpnum.dbl))
- {
- is_neg = signbit (fpnum.dbl);
- if (isupper (info->spec))
- {
- special = "INF";
- wspecial = L"INF";
- }
- else
- {
- special = "inf";
- wspecial = L"inf";
- }
- }
- else
- {
- p.fracsize = __mpn_extract_double (fp_input,
- (sizeof (fp_input)
- / sizeof (fp_input[0])),
- &p.exponent, &is_neg, fpnum.dbl);
- to_shift = 1 + p.fracsize * BITS_PER_MP_LIMB - DBL_MANT_DIG;
- }
- }
+#undef PRINTF_FP_FETCH
if (special)
{
diff --git a/stdio-common/printf_fphex.c b/stdio-common/printf_fphex.c
index b207e00..3a25854 100644
--- a/stdio-common/printf_fphex.c
+++ b/stdio-common/printf_fphex.c
@@ -157,82 +157,52 @@ __printf_fphex (FILE *fp,
/* The decimal point character must never be zero. */
assert (*decimal != '\0' && decimalwc != L'\0');
+#define PRINTF_FPHEX_FETCH(FLOAT, VAR) \
+ { \
+ VAR = *(const FLOAT *) args[0]; \
+ \
+ /* Check for special values: not a number or infinity. */ \
+ if (isnan (VAR)) \
+ { \
+ if (isupper (info->spec)) \
+ { \
+ special = "NAN"; \
+ wspecial = L"NAN"; \
+ } \
+ else \
+ { \
+ special = "nan"; \
+ wspecial = L"nan"; \
+ } \
+ } \
+ else \
+ { \
+ if (isinf (VAR)) \
+ { \
+ if (isupper (info->spec)) \
+ { \
+ special = "INF"; \
+ wspecial = L"INF"; \
+ } \
+ else \
+ { \
+ special = "inf"; \
+ wspecial = L"inf"; \
+ } \
+ } \
+ } \
+ negative = signbit (VAR); \
+ }
/* Fetch the argument value. */
#ifndef __NO_LONG_DOUBLE_MATH
if (info->is_long_double && sizeof (long double) > sizeof (double))
- {
- fpnum.ldbl = *(const long double *) args[0];
-
- /* Check for special values: not a number or infinity. */
- if (isnan (fpnum.ldbl))
- {
- if (isupper (info->spec))
- {
- special = "NAN";
- wspecial = L"NAN";
- }
- else
- {
- special = "nan";
- wspecial = L"nan";
- }
- }
- else
- {
- if (isinf (fpnum.ldbl))
- {
- if (isupper (info->spec))
- {
- special = "INF";
- wspecial = L"INF";
- }
- else
- {
- special = "inf";
- wspecial = L"inf";
- }
- }
- }
- negative = signbit (fpnum.ldbl);
- }
+ PRINTF_FPHEX_FETCH (long double, fpnum.ldbl)
else
-#endif /* no long double */
- {
- fpnum.dbl.d = *(const double *) args[0];
+#endif
+ PRINTF_FPHEX_FETCH (double, fpnum.dbl.d)
- /* Check for special values: not a number or infinity. */
- if (isnan (fpnum.dbl.d))
- {
- if (isupper (info->spec))
- {
- special = "NAN";
- wspecial = L"NAN";
- }
- else
- {
- special = "nan";
- wspecial = L"nan";
- }
- }
- else
- {
- if (isinf (fpnum.dbl.d))
- {
- if (isupper (info->spec))
- {
- special = "INF";
- wspecial = L"INF";
- }
- else
- {
- special = "inf";
- wspecial = L"inf";
- }
- }
- }
- negative = signbit (fpnum.dbl.d);
- }
+#undef PRINTF_FPHEX_FETCH
if (special)
{
--
2.4.11