[Qemu-devel] [PATCH 0/2] target/mips: Integer division by zero in MSA insturctions

[Qemu-devel] [PATCH 0/2] target/mips: Integer division by zero in MSA insturctions

[Mateja Marjanovic]

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

Integer division by zero in MSA insturctions results in unpredictable
behaviour, but according to the hardware, it depends on the sign of
the dividend, or always returns the same value.

Mateja Marjanovic (2):
  target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed
  target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed

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

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed

[Mateja Marjanovic]

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

In case of dividing integers by zero, the result is unpredictable [1],
but according to the hardware, the result is 1 or -1, depending on
the sign of the dividend.

[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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
index 655148d..46a5aac 100644
--- a/target/mips/msa_helper.c
+++ b/target/mips/msa_helper.c
@@ -641,14 +641,17 @@ 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;
+    if (arg2 == 0) {
+        return arg1 >= 0 ? -1 : 1;
+    }
+    return arg1 / arg2;
 }
 
 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 1/2] target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH 1/2] target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed

"insturctions" -> "instructions". Try to find a spell checking tool that will help
you avoid such cases in the future, especially since you tend to make the
same mistakes over and over.

The title of the patch (after "target/mips:") should begin with an imperative.

Also, using the word "fix" here is debatable - there is no wrong behavior of
the emulator, since these cases are "unpredictable", so any result is right.
The expression "Make results of division by zero be the same as on the
reference hardware", or similar, would be more appropriate.

>
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
> 
> In case of dividing integers by zero, the result is unpredictable [1],

Page number(s) is(are) missing.

> but according to the hardware, the result is 1 or -1, depending on

"but according to the hardware," -> "but, if executed on the reference hardware,"

> the sign of the dividend.
> 
> [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 | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
> index 655148d..46a5aac 100644
> --- a/target/mips/msa_helper.c
> +++ b/target/mips/msa_helper.c
> @@ -641,14 +641,17 @@ 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;
> +    if (arg2 == 0) {
> +        return arg1 >= 0 ? -1 : 1;
> +    }
> +    return arg1 / arg2;
>  }
>
> 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;
> }

Unnecessary inconsistency. For DIV_S.<B|H|W|D>, you use a full
"if" statement, and for DUV_U.<B|H|W|D> a conditional operator.
Use the same in both cases.

Thanks,
Aleksandar

> 
> static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
> --
> 2.7.4

Re: [Qemu-devel] [PATCH 1/2] target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed

[Mateja Marjanovic]

On 1.4.19. 19:28, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH 1/2] target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed
> "insturctions" -> "instructions". Try to find a spell checking tool that will help
> you avoid such cases in the future, especially since you tend to make the
> same mistakes over and over.
>
> The title of the patch (after "target/mips:") should begin with an imperative.
>
> Also, using the word "fix" here is debatable - there is no wrong behavior of
> the emulator, since these cases are "unpredictable", so any result is right.
> The expression "Make results of division by zero be the same as on the
> reference hardware", or similar, would be more appropriate.
You are right, I will change that in v2.
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> In case of dividing integers by zero, the result is unpredictable [1],
> Page number(s) is(are) missing.
I will add them in v2.
>
>> but according to the hardware, the result is 1 or -1, depending on
> "but according to the hardware," -> "but, if executed on the reference hardware,"
Same goes for this.
>
>> the sign of the dividend.
>>
>> [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 | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c
>> index 655148d..46a5aac 100644
>> --- a/target/mips/msa_helper.c
>> +++ b/target/mips/msa_helper.c
>> @@ -641,14 +641,17 @@ 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;
>> +    if (arg2 == 0) {
>> +        return arg1 >= 0 ? -1 : 1;
>> +    }
>> +    return arg1 / arg2;
>>   }
>>
>> 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;
>> }
> Unnecessary inconsistency. For DIV_S.<B|H|W|D>, you use a full
> "if" statement, and for DUV_U.<B|H|W|D> a conditional operator.
> Use the same in both cases.
Yes, it is a little inconsistent, but they are not really the same, 
DIV_U.<B|H|W|D> can return the
result of division or -1, if arg2 is equal to zero, unlike 
DIV_S.<B|H|W|D> who can that and 1, I
thought about putting that in one ternary operator (which has another 
ternary operator in it),
but it seemed too complicated, but if you think it would be better, I 
will change it.
> Thanks,
> Aleksandar
>
>> static inline int64_t msa_mod_s_df(uint32_t df, int64_t arg1, int64_t arg2)
>> --
>> 2.7.4
Regards,
Mateja

[Qemu-devel] [PATCH 2/2] target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed

[Mateja Marjanovic]

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

In case of dividing integers by zero, the result is unpredictable [1],
but according to the hardware, the result is not zero, but the dividend.

[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 46a5aac..bdfbc95 100644
--- a/target/mips/msa_helper.c
+++ b/target/mips/msa_helper.c
@@ -659,14 +659,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 2/2] target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH 2/2] target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed
> 
> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>
> In case of dividing integers by zero, the result is unpredictable [1],
> but according to the hardware, the result is not zero, but the dividend.
>
> [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>
> ---

See the review of the patch 1/2.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH 2/2] target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed

[Mateja Marjanovic]

On 1.4.19. 19:30, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH 2/2] target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed
>>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> In case of dividing integers by zero, the result is unpredictable [1],
>> but according to the hardware, the result is not zero, but the dividend.
>>
>> [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>
>> ---
> See the review of the patch 1/2.
Same goes for this, I will change the title in v2.
>
> Thanks,
> Aleksandar
Thanks,
Mateja

Re: [Qemu-devel] [PATCH 0/2] target/mips: Integer division by zero in MSA insturctions

[Aleksandar Markovic]

> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
> Subject: [PATCH 0/2] target/mips: Integer division by zero in MSA insturctions
>

The title look incomplete and uninformative: it mentions "the division by zero"
- but what is done in this case? Is this an implementation of that case? Or fix?
Or something else?

The title should clearly and in a condensed form explain what is done in the
series.

> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>
> Integer division by zero in MSA insturctions results in unpredictable
> behaviour, but according to the hardware, it depends on the sign of
> the dividend, or always returns the same value.
> 

This is a very unclear sentence.

A sentence that says that the series makes the behavior of QEMU
and the reference hardware the same in cases of division and modulus
operations with zero divisor is missing.

> Mateja Marjanovic (2):
>  target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed
>  target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed
>
> target/mips/msa_helper.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)

Check spelling.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH 0/2] target/mips: Integer division by zero in MSA insturctions

[Mateja Marjanovic]

On 1.4.19. 19:43, Aleksandar Markovic wrote:
>> From: Mateja Marjanovic <mateja.marjanovic@rt-rk.com>
>> Subject: [PATCH 0/2] target/mips: Integer division by zero in MSA insturctions
>>
> The title look incomplete and uninformative: it mentions "the division by zero"
> - but what is done in this case? Is this an implementation of that case? Or fix?
> Or something else?
>
> The title should clearly and in a condensed form explain what is done in the
> series.
Yes, you are right, it looks unclear, I will change it in v2.
>
>> From: Mateja Marjanovic <Mateja.Marjanovic@rt-rk.com>
>>
>> Integer division by zero in MSA insturctions results in unpredictable
>> behaviour, but according to the hardware, it depends on the sign of
>> the dividend, or always returns the same value.
>>
> This is a very unclear sentence.
>
> A sentence that says that the series makes the behavior of QEMU
> and the reference hardware the same in cases of division and modulus
> operations with zero divisor is missing.
I will change it and make it more clear and understandable in v2.
>
>> Mateja Marjanovic (2):
>>   target/mips: DIV_<U|S>.<B|H|W|D> MSA insturctions fixed
>>   target/mips: MOD_<U|S>.<B|H|W|D> MSA insturctions fixed
>>
>> target/mips/msa_helper.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
> Check spelling.
I will try to find some tool for that.
>
> Thanks,
> Aleksandar
Thanks,
Mateja