[Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Dmitry Poletaev]

I executed test-i386 from tests folder from QEMU rep and according to the result, instructions fistl and fistpll returns maximum positive result (0x7fff...), if a FPU register stores a positive infinity, and minimum negative result (0x80...), if a negative infinity stores in a register. Real processor in both cases returns minimum negative result (0x80...). An Intel manual(Vol. 2A 3-299 (http://www.intel.ru/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-2a-manual.pdf)) tells us "If the invalid-operation exception is masked, the integer indefinite value is stored in memory." I not found what "integer indefinite value" obviously means, but I make conclusion that "integer indefinite value" is 0x8000...

This patch fixes behaviour of fistl/fistpll instructions correct, and at least not adds new bugs(according to tcg tests), but I am not shure it doesn't breaks anything.

From: Dmitry Poletaev <poletaev-qemu@yandex.ru>
Signed-off-by: Dmitry Poletaev <poletaev-qemu@yandex.ru>

---
 fpu/softfloat.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 9274ebf..580c322 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -133,7 +133,7 @@ static int32 roundAndPackInt32( flag zSign, uint64_t absZ STATUS_PARAM)
     if ( zSign ) z = - z;
     if ( ( absZ>>32 ) || ( z && ( ( z < 0 ) ^ zSign ) ) ) {
         float_raise( float_flag_invalid STATUS_VAR);
-        return zSign ? (int32_t) 0x80000000 : 0x7FFFFFFF;
+        return (int32_t) 0x80000000;
     }
     if ( roundBits ) STATUS(float_exception_flags) |= float_flag_inexact;
     return z;
@@ -4681,12 +4681,6 @@ int64 floatx80_to_int64( floatx80 a STATUS_PARAM )
     if ( shiftCount <= 0 ) {
         if ( shiftCount ) {
             float_raise( float_flag_invalid STATUS_VAR);
-            if (    ! aSign
-                 || (    ( aExp == 0x7FFF )
-                      && ( aSig != LIT64( 0x8000000000000000 ) ) )
-               ) {
-                return LIT64( 0x7FFFFFFFFFFFFFFF );
-            }
             return (int64_t) LIT64( 0x8000000000000000 );
         }
         aSigExtra = 0;
-- 
1.8.4.msysgit.0

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Peter Maydell]

On 14 July 2014 15:35, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
> I executed test-i386 from tests folder from QEMU rep and according to the result, instructions fistl and fistpll returns maximum positive result (0x7fff...), if a FPU register stores a positive infinity, and minimum negative result (0x80...), if a negative infinity stores in a register. Real processor in both cases returns minimum negative result (0x80...). An Intel manual(Vol. 2A 3-299 (http://www.intel.ru/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-2a-manual.pdf)) tells us "If the invalid-operation exception is masked, the integer indefinite value is stored in memory." I not found what "integer indefinite value" obviously means, but I make conclusion that "integer indefinite value" is 0x8000...
>
> This patch fixes behaviour of fistl/fistpll instructions correct, and at least not adds new bugs(according to tcg tests), but I am not shure it doesn't breaks anything.

Yeah, this seems pretty likely to break all our other architectures
that use softfloat. (ARM at least gives MAXINT for +infinity
and MININT for -infinity.) This needs to either be done in the
target-i386/ code or by adding an option flag of some sort
to softfloat to permit the i386 code to specify the required
behaviour. (I checked the IEEE 754 spec and it looks like the
result value isn't specified for overflows, so this is all going
to be architecture-specific.)

The "integer indefinite value" is defined in volume 1 of the
Intel Architecture Software Developer's Manual, in table 4-1
(and also section 8.2.1); it is indeed a value with only the
most significant bit set.

I think that given that the manual says this is only done for
FIST/FISTP I would start by trying to deal with this in the
target-i386 code for those insns: call the softfloat conversion
function as usual, but if it sets the invalid flag then ignore
what softfloat returns you and use the integer-indefinite-value
instead. (Since softfloat's status flags are sticky you may need
to do a save-old-flag-value/clear-flags/softfloat op/check-flags/
merge-flag-values dance.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Dmitry Poletaev]

14.07.2014, 18:59, "Peter Maydell" <peter.maydell@linaro.org>:

>  Since softfloat's status flags are sticky ...

What does it mean?

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Peter Maydell]

On 23 July 2014 12:55, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
> 14.07.2014, 18:59, "Peter Maydell" <peter.maydell@linaro.org>:
>
>>  Since softfloat's status flags are sticky ...
>
> What does it mean?

"Sticky" here means that the status flags accumulate the
status from a sequence of operations: a softfloat function
will set the flag if the relevant exception occurred, but if
the exceptional condition did not happen then the flag will
be left at whatever its preceding value was. So you can't
just say "if the flag is set then the last operation I did set
it", because it might have been set by some operation
before that. (That is, once a bit gets set in the flags word
it "sticks" and doesn't go away.)

This matches the IEEE mandated behaviour for
floating point exception flags, which is why we do it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Dmitry Poletaev]

I'm understood. So, am I right?

From: Dmitry Poletaev <poletaev-qemu@yandex.ru>
Signed-off-by: Dmitry Poletaev <poletaev-qemu@yandex.ru>

---
 target-i386/fpu_helper.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index 1b2900d..c4fdad8 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -251,16 +251,31 @@ int32_t helper_fist_ST0(CPUX86State *env)
 int32_t helper_fistl_ST0(CPUX86State *env)
 {
     int32_t val;
-
+    signed char old_exp_flags;
+    
+    old_exp_flags = env->fp_status.float_exception_flags;
+    env->fp_status.float_exception_flags = 0;
     val = floatx80_to_int32(ST0, &env->fp_status);
+    if (env->fp_status.float_exception_flags & FPUS_IE) {
+        val = 0x80000000;
+    }
+    env->fp_status.float_exception_flags |= old_exp_flags;
     return val;
 }
 
 int64_t helper_fistll_ST0(CPUX86State *env)
 {
     int64_t val;
-
-    val = floatx80_to_int64(ST0, &env->fp_status);
+    signed char old_exp_flags;
+    
+    old_exp_flags = env->fp_status.float_exception_flags;
+    env->fp_status.float_exception_flags = 0;
+
+    val = floatx80_to_int64(ST0, &env->fp_status);    
+    if (env->fp_status.float_exception_flags & FPUS_IE) {
+        val = 0x8000000000000000;
+    }
+    env->fp_status.float_exception_flags |= old_exp_flags;
     return val;
 }
 
-- 
1.8.4.msysgit.0



23.07.2014, 16:42, "Peter Maydell" <peter.maydell@linaro.org>:
> On 23 July 2014 12:55, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
>>  14.07.2014, 18:59, "Peter Maydell" <peter.maydell@linaro.org>:
>>>   Since softfloat's status flags are sticky ...
>>  What does it mean?
>
> "Sticky" here means that the status flags accumulate the
> status from a sequence of operations: a softfloat function
> will set the flag if the relevant exception occurred, but if
> the exceptional condition did not happen then the flag will
> be left at whatever its preceding value was. So you can't
> just say "if the flag is set then the last operation I did set
> it", because it might have been set by some operation
> before that. (That is, once a bit gets set in the flags word
> it "sticks" and doesn't go away.)
>
> This matches the IEEE mandated behaviour for
> floating point exception flags, which is why we do it.
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Peter Maydell]

On 23 July 2014 16:04, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
> I'm understood. So, am I right?

Pretty much, except it's better to use the accessor functions
get_float_exception_flags() and set_float_exception_flags().

> +    if (env->fp_status.float_exception_flags & FPUS_IE) {
> +        val = 0x8000000000000000;

Also this constant needs a "ULL" suffix or it won't build on
32 bit hosts.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Dmitry Poletaev]

23.07.2014, 21:13, "Peter Maydell" <peter.maydell@linaro.org>:
>  On 23 July 2014 16:04, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
>>   I'm understood. So, am I right?
>  Pretty much, except it's better to use the accessor functions
>  get_float_exception_flags() and set_float_exception_flags().
>>   +    if (env->fp_status.float_exception_flags & FPUS_IE) {
>>   +        val = 0x8000000000000000;
>  Also this constant needs a "ULL" suffix or it won't build on
>  32 bit hosts.
>
>  thanks
>  -- PMM

Done.

From: Dmitry Poletaev <poletaev-qemu@yandex.ru>
Signed-off-by: Dmitry Poletaev <poletaev-qemu@yandex.ru>

---
 target-i386/fpu_helper.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index 1b2900d..e3543a8 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -251,16 +251,34 @@ int32_t helper_fist_ST0(CPUX86State *env)
 int32_t helper_fistl_ST0(CPUX86State *env)
 {
     int32_t val;
-
+    signed char old_exp_flags;
+    
+    old_exp_flags = get_float_exception_flags(&env->fp_status);
+    set_float_exception_flags(0, &env->fp_status);
+    
     val = floatx80_to_int32(ST0, &env->fp_status);
+    if (get_float_exception_flags(&env->fp_status) & FPUS_IE) {
+        val = 0x80000000;
+    }
+    set_float_exception_flags(get_float_exception_flags(&env->fp_status)
+                                | old_exp_flags, &env->fp_status);
     return val;
 }
 
 int64_t helper_fistll_ST0(CPUX86State *env)
 {
     int64_t val;
-
-    val = floatx80_to_int64(ST0, &env->fp_status);
+    signed char old_exp_flags;
+    
+    old_exp_flags = get_float_exception_flags(&env->fp_status);
+    set_float_exception_flags(0, &env->fp_status);
+    
+    val = floatx80_to_int32(ST0, &env->fp_status);
+    if (get_float_exception_flags(&env->fp_status) & FPUS_IE) {
+        val = 0x8000000000000000ULL;
+    }
+    set_float_exception_flags(get_float_exception_flags(&env->fp_status)
+                                | old_exp_flags, &env->fp_status);
     return val;
 }
 
-- 
1.8.4.msysgit.0

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Richard Henderson]

On 07/23/2014 05:04 AM, Dmitry Poletaev wrote:
> +    if (env->fp_status.float_exception_flags & FPUS_IE) {

Mixing bit masks.  s/FPUS_IE/float_status_invalid/


r~

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Dmitry Poletaev]

What do you mean?

29.07.2014, 23:07, "Richard Henderson" <rth@twiddle.net>:
> On 07/23/2014 05:04 AM, Dmitry Poletaev wrote:
>>  +    if (env->fp_status.float_exception_flags & FPUS_IE) {
>
> Mixing bit masks.  s/FPUS_IE/float_status_invalid/
>
> r~

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Peter Maydell]

On 14 October 2014 12:38, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
> 29.07.2014, 23:07, "Richard Henderson" <rth@twiddle.net>:
>> On 07/23/2014 05:04 AM, Dmitry Poletaev wrote:
>>>  +    if (env->fp_status.float_exception_flags & FPUS_IE) {
>>
>> Mixing bit masks.  s/FPUS_IE/float_status_invalid/

[Please don't top-post.]

> What do you mean?

Richard means that the set of defined flags for
float_exception_flags does not include "FPUS_IE"
(which is defining a symbolic constant for a bit
in an x86 register). You want to use "float_flag_invalid".
(The two happen to have the same value, 1, which is why
your code worked by accident.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Dmitry Poletaev]

From: Dmitry Poletaev <poletaev-qemu@yandex.ru>
Signed-off-by: Dmitry Poletaev <poletaev-qemu@yandex.ru>

---
 target-i386/fpu_helper.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index ab19b71..fc25a03 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -251,16 +251,34 @@ int32_t helper_fist_ST0(CPUX86State *env)
 int32_t helper_fistl_ST0(CPUX86State *env)
 {
     int32_t val;
-
+    signed char old_exp_flags;
+    
+    old_exp_flags = get_float_exception_flags(&env->fp_status);
+    set_float_exception_flags(0, &env->fp_status);
+    
     val = floatx80_to_int32(ST0, &env->fp_status);
+    if (get_float_exception_flags(&env->fp_status) & float_flag_invalid) {
+        val = 0x80000000;
+    }
+    set_float_exception_flags(get_float_exception_flags(&env->fp_status)
+                                | old_exp_flags, &env->fp_status);
     return val;
 }

 int64_t helper_fistll_ST0(CPUX86State *env)
 {
     int64_t val;
-
-    val = floatx80_to_int64(ST0, &env->fp_status);
+    signed char old_exp_flags;
+    
+    old_exp_flags = get_float_exception_flags(&env->fp_status);
+    set_float_exception_flags(0, &env->fp_status);
+    
+    val = floatx80_to_int32(ST0, &env->fp_status);
+    if (get_float_exception_flags(&env->fp_status) & float_flag_invalid) {
+        val = 0x8000000000000000ULL;
+    }
+    set_float_exception_flags(get_float_exception_flags(&env->fp_status)
+                                | old_exp_flags, &env->fp_status);
     return val;
 }

-- 
1.8.4.msysgit.0

14.10.2014, 15:56, "Peter Maydell" <peter.maydell@linaro.org>:
> On 14 October 2014 12:38, Dmitry Poletaev <poletaev-qemu@yandex.ru> wrote:
>>  29.07.2014, 23:07, "Richard Henderson" <rth@twiddle.net>:
>>>  On 07/23/2014 05:04 AM, Dmitry Poletaev wrote:
>>>>   +    if (env->fp_status.float_exception_flags & FPUS_IE) {
>>>  Mixing bit masks.  s/FPUS_IE/float_status_invalid/
>
> [Please don't top-post.]
>>  What do you mean?
>
> Richard means that the set of defined flags for
> float_exception_flags does not include "FPUS_IE"
> (which is defining a symbolic constant for a bit
> in an x86 register). You want to use "float_flag_invalid".
> (The two happen to have the same value, 1, which is why
> your code worked by accident.)
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] target-i386/FPU: wrong conversion infinity from float80 to int32/int64

[Richard Henderson]

On 11/11/2014 01:29 PM, Dmitry Poletaev wrote:
> From: Dmitry Poletaev <poletaev-qemu@yandex.ru>
> Signed-off-by: Dmitry Poletaev <poletaev-qemu@yandex.ru>
> 
> ---
>  target-i386/fpu_helper.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~