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 <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.
You are right, I will change that in v2.
> > From: Mateja Marjanovic <address@hidden>
> >
> > 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 <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.
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