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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

On Fri, Jul 22, 2016 at 10:59:18AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> 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 <nikunj@linux.vnet.ibm.com>
> >
> > 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?  If the latter, then why do you
care about those cases?

> >> +        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.

> 
> >> +    } else {
> >> +        tcg_gen_movi_i32(t2, 0);
> >> +    }
> >> +    gen_set_label(l2);
> >> +    tcg_gen_extu_i32_tl(ret, t2);
> 
> ... Here.
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]