Re: [Qemu-devel] [RFC v1 05/13] target-ppc: add modulo word operations

[David Gibson]

On Fri, Jul 22, 2016 at 12:24:55PM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
> > [ Unknown signature status ]
> > On Fri, Jul 22, 2016 at 10:59:18AM +0530, Nikunj A Dadhania wrote:
> >> David Gibson <address@hidden> writes:
> >> 
> >> > [ Unknown signature status ]
> >> > On Mon, Jul 18, 2016 at 10:35:09PM +0530, Nikunj A Dadhania wrote:
> >> >> Adding following instructions:
> >> >> 
> >> >> moduw: Modulo Unsigned Word
> >> >> modsw: Modulo Signed Word
> >> >> 
> >> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
> >> >
> >> > As rth has already mentioned this many branches probably means this
> >> > wants a helper.
> >> >
> >> >> ---
> >> >>  target-ppc/translate.c | 48 
> >> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 48 insertions(+)
> >> >> 
> >> >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> >> >> index d44f7af..487dd94 100644
> >> >> --- a/target-ppc/translate.c
> >> >> +++ b/target-ppc/translate.c
> >> >> @@ -1178,6 +1178,52 @@ GEN_DIVE(divde, divde, 0);
> >> >>  GEN_DIVE(divdeo, divde, 1);
> >> >>  #endif
> >> >>  
> >> >> +static inline void gen_op_arith_modw(DisasContext *ctx, TCGv ret, TCGv 
> >> >> arg1,
> >> >> +                                     TCGv arg2, int sign)
> >> >> +{
> >> >> +    TCGLabel *l1 = gen_new_label();
> >> >> +    TCGLabel *l2 = gen_new_label();
> >> >> +    TCGv_i32 t0 = tcg_temp_local_new_i32();
> >> >> +    TCGv_i32 t1 = tcg_temp_local_new_i32();
> >> >> +    TCGv_i32 t2 = tcg_temp_local_new_i32();
> >> >> +
> >> >> +    tcg_gen_trunc_tl_i32(t0, arg1);
> >> >> +    tcg_gen_trunc_tl_i32(t1, arg2);
> >> >> +    tcg_gen_brcondi_i32(TCG_COND_EQ, t1, 0, l1);
> >> 
> >> Result for:
> >> <anything> % 0 and ...
> >> 
> >> >> +    if (sign) {
> >> >> +        TCGLabel *l3 = gen_new_label();
> >> >> +        tcg_gen_brcondi_i32(TCG_COND_NE, t1, -1, l3);
> >> >> +        tcg_gen_brcondi_i32(TCG_COND_EQ, t0, INT32_MIN, l1);
> >> >> +        gen_set_label(l3);
> >> >
> >> > It's not really clear to be what the logic above is doing.
> >> 
> >> ... For signed case
> >> 0x8000_0000 % -1
> >> 
> >> Is undefined, addressing those cases.
> >
> > Do you mean the tcg operations have undefined results or that the ppc
> > instructions have undefined results?
> 
> TCG side, I haven't tried.
> 
> > If the latter, then why do you care about those cases?
> 
> Thats how divd is implemented as well, i didn't want to break that. I am
> looking at doing both div and mod as helpers.
> 
> >> >> +        tcg_gen_rem_i32(t2, t0, t1);
> >> >> +    } else {
> >> >> +        tcg_gen_remu_i32(t2, t0, t1);
> >> >> +    }
> >> >> +    tcg_gen_br(l2);
> >> >> +    gen_set_label(l1);
> >> >> +    if (sign) {
> >> >> +        tcg_gen_sari_i32(t2, t0, 31);
> >> >
> >> > AFAICT this sets t2 to either 0 or -1 depending on the sign of t0,
> >> > which seems like an odd thing to do.
> >> 
> >> Extending the sign later ...
> >
> > Right, so after sign extension you have a 64-bit 0 or -1.  Still not
> > seeing what that 0 or -1 result is useful for.
> 
> Oh ok, i got why you got confused. I am re-writing all of it though, but
> for understanding:
> 
>   if (divisor == 0)
>      goto l1;
> 
>   if (signed) {
>      if (divisor == -1 && dividend == INT_MIN)
>         goto l1;
>      compute_signed_rem(t2, t0, t1);
>   } else {
>      compute_unsigned_rem(t2, t0, t1);  
>   }
>   goto l2; /* jump to setting extending result and return */
> 
> l1: /* in case of invalid input set values */
>   if (signed)
>      t2 = -1 or 0;
>   else
>      t2 = 0;

Ok, so why do you ever need different result values in the case of
invalid input?  Why is always returning 0 not good enough?

> l2:
>   set (ret, t2)
> 
> Regards
> Nikunj
> 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature