| Message ID | 1554129501-27175-2-git-send-email-mateja.marjanovic@rt-rk.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/mips: Integer division by zero in MSA insturctions | expand |
> 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
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
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)