diff mbox series

[v2,01/26] s390x/tcg: Fix FP CONVERT TO (LOGICAL) FIXED NaN handling

Message ID 20210517142739.38597-2-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series s390x/tcg: Implement Vector enhancements facility and switch to z14 | expand

Commit Message

David Hildenbrand May 17, 2021, 2:27 p.m. UTC 
In case we encounter a NaN, we have to return the smallest possible
number, corresponding to either 0 or the maximum negative number. This
seems to differ from IEEE handling as implemented in softfloat, whereby
we return the biggest possible number.

While at it, use float32_to_uint64() in the CLGEB handler.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/fpu_helper.c     | 41 +++++++++++++++++++++++++++++++----
 target/s390x/vec_fpu_helper.c |  6 +++++
 2 files changed, 43 insertions(+), 4 deletions(-)

Comments

Richard Henderson June 1, 2021, 9:27 p.m. UTC | #1 
On 5/17/21 7:27 AM, David Hildenbrand wrote:
> @@ -634,6 +664,9 @@ uint64_t HELPER(clfxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
>  
>      s390_restore_bfp_rounding_mode(env, old_mode);
>      handle_exceptions(env, xxc_from_m34(m34), GETPC());
> +    if (float128_is_any_nan(make_float128(h, l))) {
> +        return 0;
> +    }

I wonder if handle_exceptions should return s390_exc.
Then you can test

   exc = handle_exceptions(...);
   if (unlikely(exc & S390_IEEE_MASK_INVALID)) {
     ret = 0;
   }
   return ret;



> +++ b/target/s390x/vec_fpu_helper.c
> @@ -326,6 +326,9 @@ void HELPER(gvec_vcdlg64s)(void *v1, const void *v2, CPUS390XState *env,
>   
>   static uint64_t vcgd64(uint64_t a, float_status *s)
>   {
> +    if (float64_is_any_nan(a)) {
> +        return INT64_MIN;
> +    }
>       return float64_to_int64(a, s);
>   }
>   
> @@ -349,6 +352,9 @@ void HELPER(gvec_vcgd64s)(void *v1, const void *v2, CPUS390XState *env,
>   
>   static uint64_t vclgd64(uint64_t a, float_status *s)
>   {
> +    if (float64_is_any_nan(a)) {
> +        return 0;
> +    }
>       return float64_to_uint64(a, s);
>   }

You do still need to raise invalid, as far as I can see.


r~
David Hildenbrand June 2, 2021, 9:50 a.m. UTC | #2 
On 01.06.21 23:27, Richard Henderson wrote:
> On 5/17/21 7:27 AM, David Hildenbrand wrote:
>> @@ -634,6 +664,9 @@ uint64_t HELPER(clfxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
>>   
>>       s390_restore_bfp_rounding_mode(env, old_mode);
>>       handle_exceptions(env, xxc_from_m34(m34), GETPC());
>> +    if (float128_is_any_nan(make_float128(h, l))) {
>> +        return 0;
>> +    }
> 
> I wonder if handle_exceptions should return s390_exc.
> Then you can test
> 
>     exc = handle_exceptions(...);
>     if (unlikely(exc & S390_IEEE_MASK_INVALID)) {
>       ret = 0;
>     }
>     return ret;
> 

I'll give it a thought if that makes things easier.

> 
> 
>> +++ b/target/s390x/vec_fpu_helper.c
>> @@ -326,6 +326,9 @@ void HELPER(gvec_vcdlg64s)(void *v1, const void *v2, CPUS390XState *env,
>>    
>>    static uint64_t vcgd64(uint64_t a, float_status *s)
>>    {
>> +    if (float64_is_any_nan(a)) {
>> +        return INT64_MIN;
>> +    }
>>        return float64_to_int64(a, s);
>>    }
>>    
>> @@ -349,6 +352,9 @@ void HELPER(gvec_vcgd64s)(void *v1, const void *v2, CPUS390XState *env,
>>    
>>    static uint64_t vclgd64(uint64_t a, float_status *s)
>>    {
>> +    if (float64_is_any_nan(a)) {
>> +        return 0;
>> +    }
>>        return float64_to_uint64(a, s);
>>    }
> 
> You do still need to raise invalid, as far as I can see.

Good point, so maybe


uint64_t ret = float64_to_uint64(a, s);

/* Note: check after converting to properly raise exceptions. */
if (float64_is_any_nan(a)) {
	ret = 0;
}
return ret;


to minimize manual handling?
David Hildenbrand June 7, 2021, 8:04 a.m. UTC | #3 
On 02.06.21 11:50, David Hildenbrand wrote:
> On 01.06.21 23:27, Richard Henderson wrote:
>> On 5/17/21 7:27 AM, David Hildenbrand wrote:
>>> @@ -634,6 +664,9 @@ uint64_t HELPER(clfxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
>>>    
>>>        s390_restore_bfp_rounding_mode(env, old_mode);
>>>        handle_exceptions(env, xxc_from_m34(m34), GETPC());
>>> +    if (float128_is_any_nan(make_float128(h, l))) {
>>> +        return 0;
>>> +    }
>>
>> I wonder if handle_exceptions should return s390_exc.
>> Then you can test
>>
>>      exc = handle_exceptions(...);
>>      if (unlikely(exc & S390_IEEE_MASK_INVALID)) {
>>        ret = 0;
>>      }
>>      return ret;
>>
> 
> I'll give it a thought if that makes things easier.

Looking at partsN(float_to_uint), we also raise float_flag_invalid in 
other cases, for example with float_class_inf. So IIUC, your proposal 
would also set "ret = 0" in case our input is +inf, which would be wrong.
diff mbox series

Patch

diff --git a/target/s390x/fpu_helper.c b/target/s390x/fpu_helper.c
index f155bc048c..13af158748 100644
--- a/target/s390x/fpu_helper.c
+++ b/target/s390x/fpu_helper.c
@@ -509,6 +509,9 @@  uint64_t HELPER(cgeb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float32_is_any_nan(v2)) {
+        return INT64_MIN;
+    }
     return ret;
 }
 
@@ -520,6 +523,9 @@  uint64_t HELPER(cgdb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float64_is_any_nan(v2)) {
+        return INT64_MIN;
+    }
     return ret;
 }
 
@@ -532,6 +538,9 @@  uint64_t HELPER(cgxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float128_is_any_nan(v2)) {
+        return INT64_MIN;
+    }
     return ret;
 }
 
@@ -543,6 +552,9 @@  uint64_t HELPER(cfeb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float32_is_any_nan(v2)) {
+        return INT32_MIN;
+    }
     return ret;
 }
 
@@ -554,6 +566,9 @@  uint64_t HELPER(cfdb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float64_is_any_nan(v2)) {
+        return INT32_MIN;
+    }
     return ret;
 }
 
@@ -566,6 +581,9 @@  uint64_t HELPER(cfxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float128_is_any_nan(v2)) {
+        return INT32_MIN;
+    }
     return ret;
 }
 
@@ -573,12 +591,12 @@  uint64_t HELPER(cfxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
 uint64_t HELPER(clgeb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 {
     int old_mode = s390_swap_bfp_rounding_mode(env, round_from_m34(m34));
-    uint64_t ret;
-
-    v2 = float32_to_float64(v2, &env->fpu_status);
-    ret = float64_to_uint64(v2, &env->fpu_status);
+    uint64_t ret = float32_to_uint64(v2, &env->fpu_status);
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float32_is_any_nan(v2)) {
+        return 0;
+    }
     return ret;
 }
 
@@ -590,6 +608,9 @@  uint64_t HELPER(clgdb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float64_is_any_nan(v2)) {
+        return 0;
+    }
     return ret;
 }
 
@@ -601,6 +622,9 @@  uint64_t HELPER(clgxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float128_is_any_nan(make_float128(h, l))) {
+        return 0;
+    }
     return ret;
 }
 
@@ -612,6 +636,9 @@  uint64_t HELPER(clfeb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float32_is_any_nan(v2)) {
+        return 0;
+    }
     return ret;
 }
 
@@ -623,6 +650,9 @@  uint64_t HELPER(clfdb)(CPUS390XState *env, uint64_t v2, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float64_is_any_nan(v2)) {
+        return 0;
+    }
     return ret;
 }
 
@@ -634,6 +664,9 @@  uint64_t HELPER(clfxb)(CPUS390XState *env, uint64_t h, uint64_t l, uint32_t m34)
 
     s390_restore_bfp_rounding_mode(env, old_mode);
     handle_exceptions(env, xxc_from_m34(m34), GETPC());
+    if (float128_is_any_nan(make_float128(h, l))) {
+        return 0;
+    }
     return ret;
 }
 
diff --git a/target/s390x/vec_fpu_helper.c b/target/s390x/vec_fpu_helper.c
index c1564e819b..d485837930 100644
--- a/target/s390x/vec_fpu_helper.c
+++ b/target/s390x/vec_fpu_helper.c
@@ -326,6 +326,9 @@  void HELPER(gvec_vcdlg64s)(void *v1, const void *v2, CPUS390XState *env,
 
 static uint64_t vcgd64(uint64_t a, float_status *s)
 {
+    if (float64_is_any_nan(a)) {
+        return INT64_MIN;
+    }
     return float64_to_int64(a, s);
 }
 
@@ -349,6 +352,9 @@  void HELPER(gvec_vcgd64s)(void *v1, const void *v2, CPUS390XState *env,
 
 static uint64_t vclgd64(uint64_t a, float_status *s)
 {
+    if (float64_is_any_nan(a)) {
+        return 0;
+    }
     return float64_to_uint64(a, s);
 }