From: Siddhesh Poyarekar <siddhesh@sourceware.org>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org
Cc: fweimer@redhat.com, joseph@codesourcery.com
Subject: Re: [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers
Date: Wed, 23 Dec 2020 07:28:16 +0530 [thread overview]
Message-ID: <d88819ae-6734-b26b-f115-0f387f63a7e5@sourceware.org> (raw)
In-Reply-To: <ba3c3630-a697-0ec5-2637-8a3fff0e79cc@linaro.org>
On 12/23/20 3:18 AM, Adhemerval Zanella via Libc-alpha wrote:
>
>
> On 15/12/2020 11:13, Siddhesh Poyarekar via Libc-alpha wrote:
>> Add some tests for fpclassify, isnanl, isinfl and issignaling.
>> ---
>> sysdeps/x86/fpu/Makefile | 3 +-
>> sysdeps/x86/fpu/test-unnormal.c | 196 ++++++++++++++++++++++++++++++++
>> 2 files changed, 198 insertions(+), 1 deletion(-)
>> create mode 100644 sysdeps/x86/fpu/test-unnormal.c
>>
>> diff --git a/sysdeps/x86/fpu/Makefile b/sysdeps/x86/fpu/Makefile
>> index 600e42c3db..e77de56d14 100644
>> --- a/sysdeps/x86/fpu/Makefile
>> +++ b/sysdeps/x86/fpu/Makefile
>> @@ -4,11 +4,12 @@ CPPFLAGS += -I../soft-fp
>>
>> libm-support += powl_helper
>> tests += test-fenv-sse test-fenv-clear-sse test-fenv-x87 test-fenv-sse-2 \
>> - test-flt-eval-method-387 test-flt-eval-method-sse
>> + test-flt-eval-method-387 test-flt-eval-method-sse test-unnormal
>> CFLAGS-test-fenv-sse.c += -msse2 -mfpmath=sse
>> CFLAGS-test-fenv-clear-sse.c += -msse2 -mfpmath=sse
>> CFLAGS-test-fenv-sse-2.c += -msse2 -mfpmath=sse
>> CFLAGS-test-flt-eval-method-387.c += -fexcess-precision=standard -mfpmath=387
>> CFLAGS-test-flt-eval-method-sse.c += -fexcess-precision=standard -msse2 \
>> -mfpmath=sse
>> +CFLAGS-test-unnormal.c += -fsignaling-nans -std=c2x
>> endif
>
> A possibility is to hookup this tests on
> math/libm-test-{fpclassify,isnan,isinf,issignaling}.inc using the new define
> I suggested on the 4/5 part [1] so you can also check if no exceptions are being
> generated and errno is not set.
>
> It increases the tests coverage and avoid a arch-specific tests.
>
> [1] https://sourceware.org/pipermail/libc-alpha/2020-December/121004.html
OK, it will need changes to the driver.
>> diff --git a/sysdeps/x86/fpu/test-unnormal.c b/sysdeps/x86/fpu/test-unnormal.c
>> new file mode 100644
>> index 0000000000..fc65d9290f
>> --- /dev/null
>> +++ b/sysdeps/x86/fpu/test-unnormal.c
>> @@ -0,0 +1,196 @@
>> +/* Test long double classification with x86 pseudo normal numbers.
>> + Copyright (C) 2020 Free Software Foundation, Inc.
>> +
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <https://www.gnu.org/licenses/>. */
>> +
>> +#include <math.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +struct tests
>> +{
>> + const char *val;
>> + int class;
>> +} inputs[] = {
>> + /* Normal. */
>> + {"\x00\x04\x00\x00\x00\x00\x00\x00\x00\x04", FP_NAN},
>> + {"\x00\x04\x00\x00\x00\x00\x00\xf0\x00\x04", FP_NORMAL},
>> + /* Pseudo-infinite. */
>> + {"\x00\x00\x00\x00\x00\x00\x00\x00\xff\x7f", FP_NAN},
>> + {"\x00\x00\x00\x00\x00\x00\x00\x80\xff\x7f", FP_INFINITE},
>> + /* Pseudo-zero. */
>> + {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", FP_NAN},
>> + {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", FP_ZERO},
>> +};
>> +
>
> I find this quite confusing to parse the value represented. I think
> it would be way more readable to include <math_ldbl.h> and define the
> values using the ieee_long_double_shape_type 'parts' member.
That's a good idea.
> If the idea is also to check snprintf, I think it would be better to
> the tests to a different test.
The *printf already has its own test.
> Also make the inputs a 'static' variable.
OK.
>> +const char *classes[5];
>> +#define stringify(N) #N
>> +
>> +static void
>> +initialize (void)
>> +{
>> + classes[FP_NAN] = stringify(FP_NAN);
>> + classes[FP_INFINITE] = stringify(FP_INFINITY);
>> + classes[FP_ZERO] = stringify(FP_ZERO);
>> + classes[FP_SUBNORMAL] = stringify(FP_SUBNORMAL);
>> + classes[FP_NORMAL] = stringify(FP_NORMAL);
>> +}
>> +
>> +static void
>> +unnormal_str (const char *val, char *ret)
>> +{
>> + for (int i = 9; i >= 0; i--)
>> + {
>> + if (i == 7 || i == 3)
>> + *ret++ = ' ';
>> + snprintf(ret, 3, "%02x", (unsigned char) val[i]);
>> + ret += 2;
>> + }
>> +}
>> +
>> +static int
>> +test_fpclassify (void)
>> +{
>> + int ret = 0;
>> +
>> + printf ("* fpclassify tests:\n");
>
> Maybe add the verbose output only when tests is invoke with --debug
> (same for other cases).
OK.
>> + for (int i = 0; i < sizeof (inputs)/sizeof (struct tests); i++)
>> + {
>> + long double value;
>> + char buf[22];
>> +
>> + memcpy (&value, inputs[i].val, 10);
>> + unnormal_str(inputs[i].val, buf);
>> + int class = fpclassify(value);
>> +
>> + if (class != inputs[i].class)
>
> Use TEST_COMPARE.
OK.
Thanks,
Siddhesh
next prev parent reply other threads:[~2020-12-23 1:58 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-15 14:13 [PATCH 0/5] x86 pseudo-normal numbers Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 1/5] x86 long double: Support pseudo numbers in fpclassifyl Siddhesh Poyarekar
2020-12-22 18:43 ` Adhemerval Zanella
2020-12-23 1:43 ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 2/5] x86 long double: Support pseudo numbers in isnanl Siddhesh Poyarekar
2020-12-22 19:04 ` Adhemerval Zanella
2020-12-23 1:49 ` Siddhesh Poyarekar
2020-12-23 8:34 ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 3/5] Partially revert 681900d29683722b1cb0a8e565a0585846ec5a61 Siddhesh Poyarekar
2020-12-15 14:36 ` Florian Weimer
2020-12-15 15:01 ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 4/5] x86 long double: Consider pseudo numbers as signaling Siddhesh Poyarekar
2020-12-22 20:13 ` Adhemerval Zanella
2020-12-23 1:50 ` Siddhesh Poyarekar
2020-12-15 14:13 ` [PATCH 5/5] x86 long double: Add tests for pseudo normal numbers Siddhesh Poyarekar
2020-12-22 21:48 ` Adhemerval Zanella
2020-12-23 1:58 ` Siddhesh Poyarekar [this message]
2020-12-23 10:20 ` Siddhesh Poyarekar
2020-12-23 17:44 ` Adhemerval Zanella
2020-12-24 0:48 ` Siddhesh Poyarekar
2020-12-15 18:26 ` [PATCH 0/5] x86 pseudo-normal numbers Joseph Myers
2020-12-15 18:29 ` Siddhesh Poyarekar
2020-12-18 4:03 ` [PING][PATCH " Siddhesh Poyarekar
2020-12-22 7:46 ` [PING*2][PATCH " Siddhesh Poyarekar
2020-12-22 11:11 ` Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d88819ae-6734-b26b-f115-0f387f63a7e5@sourceware.org \
--to=siddhesh@sourceware.org \
--cc=adhemerval.zanella@linaro.org \
--cc=fweimer@redhat.com \
--cc=joseph@codesourcery.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).