On Mon, 22 Feb 2021, Patrick McGehearty via Gcc-patches wrote:
> diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
> index 9f993c4..7dd319c 100644
> --- a/gcc/c-family/c-cppbuiltin.c
> +++ b/gcc/c-family/c-cppbuiltin.c
> @@ -1026,10 +1026,10 @@ c_cpp_builtins (cpp_reader *pfile)
> cpp_define (pfile, "__cpp_using_enum=201907L");
> }
> if (cxx_dialect > cxx20)
> - {
> - /* Set feature test macros for C++23. */
> - cpp_define (pfile, "__cpp_size_t_suffix=202011L");
> - }
> + {
> + /* Set feature test macros for C++23. */
> + cpp_define (pfile, "__cpp_size_t_suffix=202011L");
> + }
This is a spurious whitespace change that does not belong in this patch.
> sprintf (macro_name, "__LIBGCC_%s_FUNC_EXT__", name);
> char suffix[20] = "";
> if (mode == TYPE_MODE (double_type_node))
> - ; /* Empty suffix correct. */
> + {
> + ; /* Empty suffix correct. */
> + memcpy (float_h_prefix, "DBL", 4);
No need for the empty statement any more; remove the "; " and just leave
the comment.
> diff --git a/gcc/testsuite/gcc.c-torture/execute/ieee/cdivchkld.c
> b/gcc/testsuite/gcc.c-torture/execute/ieee/cdivchkld.c
> new file mode 100644
> index 0000000..74046ee
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/ieee/cdivchkld.c
> @@ -0,0 +1,123 @@
> +/*
> + Program to test complex divide for correct results on selected values.
> + Checking known failure points.
> +*/
> +
> +extern void abort();
> +extern void exit();
In all the tests, please use prototype function declarations rather than
unprototyped. Although tests don't need to follow GNU coding style, new
tests should avoid legacy features such as unprototyped functions unless
specifically intended to test those features.
> +extern long double log2l(long double);
> +
> +#define SMALL (0x1.0p-16384L)
> +#define MAXBIT 63
> +#define ERRLIM 6
> +
> +/*
> + Compare c (computed value) with z (expected value).
> + Return 0 if within allowed range. Return 1 if not.
> +*/
> +int match(_Complex c, _Complex z)
In general, for all the tests, I don't think you should use _Complex
meaning double _Complex; you should say double _Complex explicitly.
In this test (the long double test), it looks particularly wrong, because
I'd expect this function to take long double _Complex arguments, not
double _Complex; it's probably not verifying anything useful otherwise.
This test seems very specific to one particular long double format (the
x86 "extended" format). Maybe the test still works for binary128 long
double, but it probably doesn't verify much useful in that case (because
it will only be checking for about 64 accurate bits of results rather than
for about 113 bits). On targets where long double has the same (binary64)
format as double, it might well fail; likewise IBM long double.
You could instead have #if conditionals (on the various LDBL_* constants)
to distinguish between binary128, x86/m68k "extended", IBM long double,
long double = double, other formats, and have appropriate test inputs and
expected outputs for each. Rather than hardcoding MAXBIT and SMALL
definitions, it would be better to base things on LDBL_MANT_DIG and
LDBL_MIN; then only the specific test inputs should need adjusting for the
different formats.
It will then be necessary to run this test for GCC targets using the
various different formats to make sure it works correctly for them.
Hopefully there are suitable GCC Compile Farm systems for doing so.
> + biterr = log2l(rmax) + MAXBIT + 1;
I'd tend to think it's better to use ilogbl rather than log2l, to compute
an integer exponent; likewise in the other tests.
> diff --git a/libgcc/config/rs6000/_divkc3.c b/libgcc/config/rs6000/_divkc3.c
> index d261f40..62a29cf 100644
> --- a/libgcc/config/rs6000/_divkc3.c
> +++ b/libgcc/config/rs6000/_divkc3.c
> @@ -37,31 +37,118 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
> If not, see
> #define __divkc3 __divkc3_sw
> #endif
>
> +# define RBIG ((__LIBGCC_TF_MAX__)/2)
> +# define RMIN (__LIBGCC_TF_MIN__)
> +# define RMIN2 (__LIBGCC_TF_EPSILON__)
> +# define RMINSCAL (1/__LIBGCC_TF_EPSILON__)
> +# define RMAX2 ((RBIG)*(RMIN2))
Coding style issues here:
* No space between "#" and "define"; it's not being used above.
* Space around binary operators '*' and '/'.
* No need for parentheses around the expansions of __LIBGCC_* macros, or
around RBIG and RMIN2 in the definition of RMAX2.
So RBIG should expand to (__LIBGCC_TF_MAX__ / 2), for example; RMIN should
expand to __LIBGCC_TF_MIN__; RMAX2 should expand to (RBIG * RMIN2).
Likewise for similar definitions in libgcc2.c. *But*, in libgcc2.c, the
space between "#" and "define" *is* appropriate, as it's already being
used there (to indicate that the #define is inside a #if conditional - the
defines in libgcc2.c are conditional, those in _divkc3.c aren't).
> + /* Scale by max(c,d) to reduce chances of denominator overflowing. */
> + if (FABS(c) < FABS(d))
Throughout this patch, both in this file and in libgcc2.c, there should be
a space before '(' in calls to FABS.
--
Joseph S. Myers
jos...@codesourcery.com
