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

[Nikunj A Dadhania]

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.

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