Re: [Qemu-devel] [PATCH 1/2] softfloat: Enable run-time-configurable meaning of signaling NaN bit

[Aleksandar Markovic <Aleksandar.Markovic@imgtec.com>]

Hello, Richard.

I truly appreciate your guidance, it is of tremendous help to me.

I plan to address all issues you brought up in this mail in the V2 of my patch series.

All is clear to me, except some subtleties regarding initialization of platforms.
More specifically, for setting snan bit, I adopted this principle.

1. If target platform doesn't use SoftFloatLibrary, nothing is added.
      (cris, lm32, moxie)
2. Else, if target platform's signalling bit meaning is "1 is signalling",
   explicit invocation of set_snan_bit_is_one(1) is added.
      (mips, sh4, unicore32)
         (for mips this will change in the second patch of this series,
         but this very patch leaves mips features as they currently are.)
3. Else, if target platform explicitely sets other fields of its
   float_status structure(s), explicit invocation of
   set_snan_bit_is_one(0) is added.
      (arm, ppc, s390x, tricore)
4. Else, if target platform doesn't exlpcitely sets its structure CPUXXXState,
   explicit invocation of set_snan_bit_is_one(0) is added.
      (alpha, xtensa)
5. For remaining cases, nothing is added. (Those cases explicitely set
   their CPUXXXState to 0, and implicitly perform set_snan_bit_is_one(0).)
      (i386, m68k, microblaze, openrisc, sparc)

By platforms, this looks like following:

A. alpha - structure CPUAlphaState not explicitely set to 0s,
      explicit invocation of set_snan_bit_is_one(0) added
B. arm - explicitely sets other fields of float_status,
      explicit invocation of set_snan_bit_is_one(0) added
C. cris - does not use SoftFloat library, nothing added
D. i386 - structure CPUX86State explicitely set to 0s,
      nothing added (implicit set_snan_bit_is_one(0))
E. lm32 - does not use SoftFloat library, nothing added
F. m68k - structure CPUM68KState explicitely set to 0s,
      nothing added (implicit set_snan_bit_is_one(0))
G. microblaze - structure CPUMBState explicitely set to 0s,
      nothing added (implicit set_snan_bit_is_one(0))
H. mips - must explicitely invoke set_snan_bit_is_one(1)
I. moxie - does not use SoftFloat library, nothing added
J. openrisc - structure CPUOpenRISCState explicitely set to 0s,
      nothing added (implicit set_snan_bit_is_one(0))
K. ppc - explicitely sets other fields of float_status,
      explicit invocation of set_snan_bit_is_one(0) added
L. s390x - explicitely sets other fields of float_status,
      explicit invocation of set_snan_bit_is_one(0) added
M. sh4 - must explicitely invoke set_snan_bit_is_one(1)
N. sparc - structure CPUSPARCState explicitely set to 0s,
      nothing added (implicit set_snan_bit_is_one(0))
O. tricore - explicitely sets other fields of float_status,
      explicit invocation of set_snan_bit_is_one(0) added
P. unicore32 - must explicitely invoke set_snan_bit_is_one(1)
Q. xtensa - structure CPUXtensaState not explicitely set to 0s,
      explicit invocation of set_snan_bit_is_one(0) added

Does such approach look OK to you?

Feightfully yours,
Aleksandar



________________________________________
From: qemu-devel-bounces+aleksandar.markovic=imgtec.com@nongnu.org [qemu-devel-bounces+aleksandar.markovic=imgtec.com@nongnu.org] on behalf of Richard Henderson [rth@twiddle.net]
Sent: Monday, March 28, 2016 2:36 PM
To: Aleksandar Markovic; qemu-devel@nongnu.org
Cc: peter.maydell@linaro.org; ehabkost@redhat.com; kbastian@mail.uni-paderborn.de; mark.cave-ayland@ilande.co.uk; agraf@suse.de; Petar Jovanovic; blauwirbel@gmail.com; jcmvbkbc@gmail.com; Miodrag Dinic; qemu-arm@nongnu.org; qemu-ppc@nongnu.org; edgar.iglesias@gmail.com; pbonzini@redhat.com; gxt@mprc.pku.edu.cn; Leon Alrae; afaerber@suse.de; aurelien@aurel32.net; proljc@gmail.com
Subject: Re: [Qemu-devel] [PATCH 1/2] softfloat: Enable run-time-configurable meaning of signaling NaN bit

On 03/25/2016 05:50 AM, Aleksandar Markovic wrote:
> +float16 float16_default_nan(float_status *status) {

{ on the next line.

> +        return const_float64(LIT64( 0xFFF8000000000000 ));

Let's please fix the horrible formatting in this file as we touch the lines,
please.

> -#define floatx80_default_nan_high 0x7FFF
> -#define floatx80_default_nan_low  LIT64(0xBFFFFFFFFFFFFFFF)
> -#else
> -#define floatx80_default_nan_high 0xFFFF
> -#define floatx80_default_nan_low  LIT64( 0xC000000000000000 )
> -#endif
> +uint16_t floatx80_default_nan_high(float_status *status) {
> +uint64_t floatx80_default_nan_low(float_status *status) {

Why do you need two separate functions for this?

> +floatx80 floatx80_default_nan(float_status *status) {

Seems to me this one is good enough, and indeed preferable.

> -#define float128_default_nan_high LIT64(0x7FFF7FFFFFFFFFFF)
> -#define float128_default_nan_low  LIT64(0xFFFFFFFFFFFFFFFF)
...
> +float128 float128_default_nan(float_status *status) {

Likewise.


> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index 2eab060..1714387 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -277,6 +277,8 @@ static void alpha_cpu_initfn(Object *obj)
>   #endif
>       env->lock_addr = -1;
>       env->fen = 1;
> +
> +    set_snan_bit_is_one(0, &env->fp_status);

We've just done a memset of (most of) the entire cpu struct.  We don't need to
re-initialize this field to zero here.  Same with most of the other cpus.


r~