[Qemu-devel] [PATCH v2 0/2] target/mips: Adjusting the results when dividing by zero in MSA instructions

[Qemu-devel] [PATCH v2 0/2] target/mips: Adjusting the results when dividing by zero in MSA instructions

[Mateja Marjanovic]

From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

The behaviour when executing DIV_<U|S>.<B|H|W|D> and MOD_<U|S>.<B|H|W|D>
differs on a referent hardware (FPGA MIPS 64 r6, little endian) and on
QEMU, when the divisor is equal to zero. That is not a real bug, because
the behaviour in that case is unpredictable (references in commit
messages).

v2:
 - Add consistency in DIV_U.df and DIV_S.df helper implementation
 - Adjust the commit messages and the cover letter, making them
   clearer and more understandable
 - Correct the spelling in the commit messages and in the cover
   letter

Mateja Marjanovic (2):
  target/mips: Make the results of DIV_<U|S>.<B|H|W|D> the same as on
    hardware
  target/mips: Make the results of MOD_<U|S>.<B|H|W|D> the same as on
    hardware

 target/mips/msa_helper.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/2] target/mips: Make the results of DIV_<U|S>.<B|H|W|D> the same as on hardware

[Mateja Marjanovic]

From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

MSA instructions DIV_<U|S>.<B|H|W|D> when dividing by zero,
didn't return the same value when executed on a referent hardware
(FPGA MIPS 64 r6, little endian) and when executed on QEMU, which
is not a real bug, because the result when dividing by zero is
UNPREDICTABLE [1] (page 141, 142).

[1] MIPS Architecture for Programmers
    Volume IV-j: The MIPS64 SIMD
    Architecture Module, Revision 1.12

Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
---
 target/mips/msa_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
index 655148d..8c77f12 100644
--- a/target/mips/msa_helper.c
+++ b/target/mips/msa_helper.c
@@ -641,14 +641,15 @@ static inline int64_t msa_div_s_df(uint32_t df, int64_t arg1, int64_t arg2)
     if (arg1 == DF_MIN_INT(df) && arg2 == -1) {
         return DF_MIN_INT(df);
     }
-    return arg2 ? arg1 / arg2 : 0;
+    return arg2 ? arg1 / arg2
+                : arg1 >= 0 ? -1 : 1;
 }
 
 static inline int64_t msa_div_u_df(uint32_t df, int64_t arg1, int64_t arg2)
 {
     uint64_t u_arg1 = UNSIGNED(arg1, df);
     uint64_t u_arg2 = UNSIGNED(arg2, df);
-    return u_arg2 ? u_arg1 / u_arg2 : 0;
+    return arg2 ? u_arg1 / u_arg2 : -1;
 }
 
 static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/2] target/mips: Make the results of MOD_<U|S>.<B|H|W|D> the same as on hardware

[Mateja Marjanovic]

From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>

MSA instructions MOD_<U|S>.<B|H|W|D> when dividing by zero,
didn't return the same value when executed on a referent hardware
(FPGA MIPS 64 r6, little endian) and when executed on QEMU, which
is not a real bug, because the result when dividing by zero is
UNPREDICTABLE [1] (page 255, 256).

[1] MIPS Architecture for Programmers
    Volume IV-j: The MIPS64 SIMD
    Architecture Module, Revision 1.12

Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
---
 target/mips/msa_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
index 8c77f12..6693ba6 100644
--- a/target/mips/msa_helper.c
+++ b/target/mips/msa_helper.c
@@ -657,14 +657,14 @@ static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
     if (arg1 == DF_MIN_INT(df) && arg2 == -1) {
         return 0;
     }
-    return arg2 ? arg1 % arg2 : 0;
+    return arg2 ? arg1 % arg2 : arg1;
 }
 
 static inline int64_t msa_mod_u_df(uint32_t df, int64_t arg1, int64_t arg2)
 {
     uint64_t u_arg1 = UNSIGNED(arg1, df);
     uint64_t u_arg2 = UNSIGNED(arg2, df);
-    return u_arg2 ? u_arg1 % u_arg2 : 0;
+    return u_arg2 ? u_arg1 % u_arg2 : u_arg1;
 }
 
 #define SIGNED_EVEN(a, df) \
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/2] target/mips: Make the results of MOD_<U|S>.<B|H|W|D> the same as on hardware

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v2 2/2] target/mips: Make the results of MOD_<U|S>.<B|H|W|D> the same as on hardware
> 
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>
> MSA instructions MOD_<U|S>.<B|H|W|D> when dividing by zero,
> didn't return the same value when executed on a referent hardware
> (FPGA MIPS 64 r6, little endian) and when executed on QEMU, which
> is not a real bug, because the result when dividing by zero is
> UNPREDICTABLE [1] (page 255, 256).
> 
> [1] MIPS Architecture for Programmers
>     Volume IV-j: The MIPS64 SIMD
>     Architecture Module, Revision 1.12
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

>  target/mips/msa_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
> index 8c77f12..6693ba6 100644
> --- a/target/mips/msa_helper.c
> +++ b/target/mips/msa_helper.c
> @@ -657,14 +657,14 @@ static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t > arg2)
>      if (arg1 == DF_MIN_INT(df) && arg2 == -1) {
>          return 0;
>      }
> -    return arg2 ? arg1 % arg2 : 0;
> +    return arg2 ? arg1 % arg2 : arg1;
>  }
> 
>  static inline int64_t msa_mod_u_df(uint32_t df, int64_t arg1, int64_t arg2)
>  {
>      uint64_t u_arg1 = UNSIGNED(arg1, df);
>      uint64_t u_arg2 = UNSIGNED(arg2, df);
> -    return u_arg2 ? u_arg1 % u_arg2 : 0;
> +    return u_arg2 ? u_arg1 % u_arg2 : u_arg1;
>  }
> 
>  #define SIGNED_EVEN(a, df) \
> --
> 2.7.4

Re: [Qemu-devel] [PATCH v2 1/2] target/mips: Make the results of DIV_<U|S>.<B|H|W|D> the same as on hardware

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH v2 1/2] target/mips: Make the results of DIV_<U|S>.<B|H|W|D> the same as on > hardware
> 
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> MSA instructions DIV_<U|S>.<B|H|W|D> when dividing by zero,
> didn't return the same value when executed on a referent hardware
> (FPGA MIPS 64 r6, little endian) and when executed on QEMU, which
> is not a real bug, because the result when dividing by zero is
> UNPREDICTABLE [1] (page 141, 142).
> 
> [1] MIPS Architecture for Programmers
>     Volume IV-j: The MIPS64 SIMD
>     Architecture Module, Revision 1.12
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

>  target/mips/msa_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
> index 655148d..8c77f12 100644
> --- a/target/mips/msa_helper.c
> +++ b/target/mips/msa_helper.c
> @@ -641,14 +641,15 @@ static inline int64_t msa_div_s_df(uint32_t df, int64_t arg1, int64_t > arg2)
>      if (arg1 == DF_MIN_INT(df) && arg2 == -1) {
>          return DF_MIN_INT(df);
>      }
> -    return arg2 ? arg1 / arg2 : 0;
> +    return arg2 ? arg1 / arg2
> +                : arg1 >= 0 ? -1 : 1;
>  }
> 
>  static inline int64_t msa_div_u_df(uint32_t df, int64_t arg1, int64_t arg2)
>  {
>      uint64_t u_arg1 = UNSIGNED(arg1, df);
>      uint64_t u_arg2 = UNSIGNED(arg2, df);
> -    return u_arg2 ? u_arg1 / u_arg2 : 0;
> +    return arg2 ? u_arg1 / u_arg2 : -1;
>  }
> 
>  static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
> --
> 2.7.4

Re: [Qemu-devel] [PATCH v2 1/2] target/mips: Make the results of DIV_<U|S>.<B|H|W|D> the same as on hardware

[Philippe Mathieu-Daudé]

Hi Mateja,

On 4/2/19 2:11 PM, Mateja Marjanovic wrote:
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> MSA instructions DIV_<U|S>.<B|H|W|D> when dividing by zero,
> didn't return the same value when executed on a referent hardware
> (FPGA MIPS 64 r6, little endian) and when executed on QEMU, which
> is not a real bug, because the result when dividing by zero is
> UNPREDICTABLE [1] (page 141, 142).

I'm surprised by the arch, I'd have expected a MSA floating point
exception instead of UNPREDICTABLE.

So here we decide to follow the FPGA model behavior rather than the
architecture...
If the community agree, my only request is to add a comment in the code
that this is the "FPGA MIPS 64 r6" behavior (else while looking at this
code later I'd be tempted to revert to a 0 return value instead of -1/1).

> 
> [1] MIPS Architecture for Programmers
>     Volume IV-j: The MIPS64 SIMD
>     Architecture Module, Revision 1.12
> 
> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> ---
>  target/mips/msa_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
> index 655148d..8c77f12 100644
> --- a/target/mips/msa_helper.c
> +++ b/target/mips/msa_helper.c
> @@ -641,14 +641,15 @@ static inline int64_t msa_div_s_df(uint32_t df, int64_t arg1, int64_t arg2)
>      if (arg1 == DF_MIN_INT(df) && arg2 == -1) {
>          return DF_MIN_INT(df);
>      }
> -    return arg2 ? arg1 / arg2 : 0;
> +    return arg2 ? arg1 / arg2
> +                : arg1 >= 0 ? -1 : 1;
>  }
>  
>  static inline int64_t msa_div_u_df(uint32_t df, int64_t arg1, int64_t arg2)
>  {
>      uint64_t u_arg1 = UNSIGNED(arg1, df);
>      uint64_t u_arg2 = UNSIGNED(arg2, df);
> -    return u_arg2 ? u_arg1 / u_arg2 : 0;
> +    return arg2 ? u_arg1 / u_arg2 : -1;
>  }
>  
>  static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
> 

Re: [Qemu-devel] [PATCH v2 1/2] target/mips: Make the results of DIV_<U|S>.<B|H|W|D> the same as on hardware

[Mateja Marjanovic]

On 2.4.19. 17:08, Philippe Mathieu-Daudé wrote:
> Hi Mateja,
>
> On 4/2/19 2:11 PM, Mateja Marjanovic wrote:
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> MSA instructions DIV_<U|S>.<B|H|W|D> when dividing by zero,
>> didn't return the same value when executed on a referent hardware
>> (FPGA MIPS 64 r6, little endian) and when executed on QEMU, which
>> is not a real bug, because the result when dividing by zero is
>> UNPREDICTABLE [1] (page 141, 142).
> I'm surprised by the arch, I'd have expected a MSA floating point
> exception instead of UNPREDICTABLE.

Our architecture documents do not list any exceptions related to
division by zero, for instructions DIV_<U|S>.<B|H|W|D> and
MOD_<U|S>.<B|H|W|D>.

The only exceptions that are mentioned are:
Reserved Instruction Exception, MSA Disabled Exception. [1] (page 141, 
142, 254, 255).

>
> So here we decide to follow the FPGA model behavior rather than the
> architecture...
> If the community agree, my only request is to add a comment in the code
> that this is the "FPGA MIPS 64 r6" behavior (else while looking at this
> code later I'd be tempted to revert to a 0 return value instead of -1/1).
>
>> [1] MIPS Architecture for Programmers
>>      Volume IV-j: The MIPS64 SIMD
>>      Architecture Module, Revision 1.12
>>
>> Signed-off-by: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> ---
>>   target/mips/msa_helper.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
>> index 655148d..8c77f12 100644
>> --- a/target/mips/msa_helper.c
>> +++ b/target/mips/msa_helper.c
>> @@ -641,14 +641,15 @@ static inline int64_t msa_div_s_df(uint32_t df, int64_t arg1, int64_t arg2)
>>       if (arg1 == DF_MIN_INT(df) && arg2 == -1) {
>>           return DF_MIN_INT(df);
>>       }
>> -    return arg2 ? arg1 / arg2 : 0;
>> +    return arg2 ? arg1 / arg2
>> +                : arg1 >= 0 ? -1 : 1;
>>   }
>>   
>>   static inline int64_t msa_div_u_df(uint32_t df, int64_t arg1, int64_t arg2)
>>   {
>>       uint64_t u_arg1 = UNSIGNED(arg1, df);
>>       uint64_t u_arg2 = UNSIGNED(arg2, df);
>> -    return u_arg2 ? u_arg1 / u_arg2 : 0;
>> +    return arg2 ? u_arg1 / u_arg2 : -1;
>>   }
>>   
>>   static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
Thanks,
Mateja