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

[Aleksandar Markovic]

> From: Mateja Marjanovic <address@hidden>
> 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 <address@hidden>
> 
> 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 <address@hidden>
>
> ---
> 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