diff mbox

[RFC,v1,05/13] target-ppc: add modulo word operations

Message ID 1468861517-2508-6-git-send-email-nikunj@linux.vnet.ibm.com
State New
Headers show

Commit Message

Nikunj A Dadhania July 18, 2016, 5:05 p.m. UTC
Adding following instructions:

moduw: Modulo Unsigned Word
modsw: Modulo Signed Word

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 target-ppc/translate.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

David Gibson July 22, 2016, 4:51 a.m. UTC | #1
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);
> +    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.

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

> +    } else {
> +        tcg_gen_movi_i32(t2, 0);
> +    }
> +    gen_set_label(l2);
> +    tcg_gen_extu_i32_tl(ret, t2);
> +    tcg_temp_free_i32(t0);
> +    tcg_temp_free_i32(t1);
> +    tcg_temp_free_i32(t2);
> +}
> +
> +#define GEN_INT_ARITH_MODW(name, opc3, sign)                                \
> +static void glue(gen_, name)(DisasContext *ctx)                             \
> +{                                                                           \
> +    gen_op_arith_modw(ctx, cpu_gpr[rD(ctx->opcode)],                        \
> +                      cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],   \
> +                      sign);                                                \
> +}
> +
> +GEN_INT_ARITH_MODW(modsw, 0x18, 1);
> +GEN_INT_ARITH_MODW(moduw, 0x08, 0);
> +
>  /* mulhw  mulhw. */
>  static void gen_mulhw(DisasContext *ctx)
>  {
> @@ -10244,6 +10290,8 @@ GEN_HANDLER_E(divwe, 0x1F, 0x0B, 0x0D, 0, PPC_NONE, PPC2_DIVE_ISA206),
>  GEN_HANDLER_E(divweo, 0x1F, 0x0B, 0x1D, 0, PPC_NONE, PPC2_DIVE_ISA206),
>  GEN_HANDLER_E(divweu, 0x1F, 0x0B, 0x0C, 0, PPC_NONE, PPC2_DIVE_ISA206),
>  GEN_HANDLER_E(divweuo, 0x1F, 0x0B, 0x1C, 0, PPC_NONE, PPC2_DIVE_ISA206),
> +GEN_HANDLER_E(modsw, 0x1F, 0x0B, 0x18, 0x00000001, PPC_NONE, PPC2_ISA300),
> +GEN_HANDLER_E(moduw, 0x1F, 0x0B, 0x08, 0x00000001, PPC_NONE, PPC2_ISA300),
>  
>  #if defined(TARGET_PPC64)
>  #undef GEN_INT_ARITH_DIVD
Nikunj A Dadhania July 22, 2016, 5:29 a.m. UTC | #2
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.

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

>> +    } else {
>> +        tcg_gen_movi_i32(t2, 0);
>> +    }
>> +    gen_set_label(l2);
>> +    tcg_gen_extu_i32_tl(ret, t2);

... Here.

Regards
Nikunj
David Gibson July 22, 2016, 6:09 a.m. UTC | #3
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
>
Nikunj A Dadhania July 22, 2016, 6:54 a.m. UTC | #4
David Gibson <david@gibson.dropbear.id.au> writes:

> [ Unknown signature status ]
> 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?

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;
l2:
  set (ret, t2)

Regards
Nikunj
David Gibson July 22, 2016, 7:12 a.m. UTC | #5
On Fri, Jul 22, 2016 at 12:24:55PM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > [ Unknown signature status ]
> > 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?
> 
> 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
> 
>
Nikunj A Dadhania July 22, 2016, 8 a.m. UTC | #6
David Gibson <david@gibson.dropbear.id.au> writes:

> [ Unknown signature status ]
> On Fri, Jul 22, 2016 at 12:24:55PM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > [ Unknown signature status ]
>> > 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?
>> 
>> 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?

Let me go through the spec, as divd does the same thing.

Regards
Nikunj
diff mbox

Patch

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);
+    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);
+        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);
+    } else {
+        tcg_gen_movi_i32(t2, 0);
+    }
+    gen_set_label(l2);
+    tcg_gen_extu_i32_tl(ret, t2);
+    tcg_temp_free_i32(t0);
+    tcg_temp_free_i32(t1);
+    tcg_temp_free_i32(t2);
+}
+
+#define GEN_INT_ARITH_MODW(name, opc3, sign)                                \
+static void glue(gen_, name)(DisasContext *ctx)                             \
+{                                                                           \
+    gen_op_arith_modw(ctx, cpu_gpr[rD(ctx->opcode)],                        \
+                      cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)],   \
+                      sign);                                                \
+}
+
+GEN_INT_ARITH_MODW(modsw, 0x18, 1);
+GEN_INT_ARITH_MODW(moduw, 0x08, 0);
+
 /* mulhw  mulhw. */
 static void gen_mulhw(DisasContext *ctx)
 {
@@ -10244,6 +10290,8 @@  GEN_HANDLER_E(divwe, 0x1F, 0x0B, 0x0D, 0, PPC_NONE, PPC2_DIVE_ISA206),
 GEN_HANDLER_E(divweo, 0x1F, 0x0B, 0x1D, 0, PPC_NONE, PPC2_DIVE_ISA206),
 GEN_HANDLER_E(divweu, 0x1F, 0x0B, 0x0C, 0, PPC_NONE, PPC2_DIVE_ISA206),
 GEN_HANDLER_E(divweuo, 0x1F, 0x0B, 0x1C, 0, PPC_NONE, PPC2_DIVE_ISA206),
+GEN_HANDLER_E(modsw, 0x1F, 0x0B, 0x18, 0x00000001, PPC_NONE, PPC2_ISA300),
+GEN_HANDLER_E(moduw, 0x1F, 0x0B, 0x08, 0x00000001, PPC_NONE, PPC2_ISA300),
 
 #if defined(TARGET_PPC64)
 #undef GEN_INT_ARITH_DIVD