Re: [Qemu-devel] [PATCH v4 07/15] target-ppc: implement branch-less divw[o][.]

[Nikunj A Dadhania]

David Gibson <address@hidden> writes:

> [ Unknown signature status ]
> On Wed, Jul 27, 2016 at 11:47:15AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <address@hidden> writes:
>> 
>> > [ Unknown signature status ]
>> > On Tue, Jul 26, 2016 at 05:28:30PM +0530, Nikunj A Dadhania wrote:
>> >> While implementing modulo instructions figured out that the
>> >> implementation uses many branches. Change the logic to achieve the
>> >> branch-less code. Undefined value is set to dividend in case of invalid
>> >> input.
>> >> 
>> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
>> >> ---
>> >>  target-ppc/translate.c | 48 
>> >> +++++++++++++++++++++++-------------------------
>> >>  1 file changed, 23 insertions(+), 25 deletions(-)
>> >> 
>> >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> >> index 7c7328f..69d9ae0 100644
>> >> --- a/target-ppc/translate.c
>> >> +++ b/target-ppc/translate.c
>> >> @@ -1049,41 +1049,39 @@ static void gen_addpcis(DisasContext *ctx)
>> >>  static inline void gen_op_arith_divw(DisasContext *ctx, TCGv ret, TCGv 
>> >> arg1,
>> >>                                       TCGv arg2, int sign, int compute_ov)
>> >>  {
>> >> -    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 t0 = tcg_temp_new_i32();
>> >> +    TCGv_i32 t1 = tcg_temp_new_i32();
>> >> +    TCGv_i32 t2 = tcg_temp_new_i32();
>> >> +    TCGv_i32 t3 = tcg_temp_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_div_i32(t0, t0, t1);
>> >> -    } else {
>> >> -        tcg_gen_divu_i32(t0, t0, t1);
>> >> -    }
>> >> -    if (compute_ov) {
>> >> -        tcg_gen_movi_tl(cpu_ov, 0);
>> >> -    }
>> >> -    tcg_gen_br(l2);
>> >> -    gen_set_label(l1);
>> >>      if (sign) {
>> >> -        tcg_gen_sari_i32(t0, t0, 31);
>> >> +        tcg_gen_setcondi_i32(TCG_COND_EQ, t2, t0, INT_MIN);
>> >> +        tcg_gen_setcondi_i32(TCG_COND_EQ, t3, t1, -1);
>> >> +        tcg_gen_and_i32(t2, t2, t3);
>> >> +        tcg_gen_setcondi_i32(TCG_COND_EQ, t3, t1, 0);
>> >> +        tcg_gen_or_i32(t2, t2, t3);
>> >> +        tcg_gen_movi_i32(t3, 0);
>> >> +        tcg_gen_movcond_i32(TCG_COND_NE, t1, t2, t3, t2, t1);
>> >> +        tcg_gen_div_i32(t3, t0, t1);
>> >> +        tcg_gen_extu_i32_tl(ret, t3);
>> >
>> > Should this be a signed extend, given it's a signed divide?
>> 
>> Don't think so, as the instruction is 32-bit, caller will only look at
>> the 32bit and div_i32 is signed, it will take care of extending sign
>> till 32-boundary.
>
> Hrm.  I thought most 32-bit arithmetic operations on Power actually
> set the underlying 64-bit registers to a sign extended version of the
> 32-bit result.

I think, when I want to operate on it as 64-bit, i will need signed
extension. rth can give more info on this.

Retained the behaviour as per the previous code as well:

    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_div_i32(t0, t0, t1);
    } else {
        tcg_gen_divu_i32(t0, t0, t1);
    }
    if (compute_ov) {
        tcg_gen_movi_tl(cpu_ov, 0);
    }
    tcg_gen_br(l2);
    gen_set_label(l1);
    if (sign) {
        tcg_gen_sari_i32(t0, t0, 31);
    } else {
        tcg_gen_movi_i32(t0, 0);
    }
    if (compute_ov) {
        tcg_gen_movi_tl(cpu_ov, 1);
        tcg_gen_movi_tl(cpu_so, 1);
    }
    gen_set_label(l2);
    tcg_gen_extu_i32_tl(ret, t0);                     <<<<<<<<<<<<<<<<<<<<<<<
    tcg_temp_free_i32(t0);
    tcg_temp_free_i32(t1);

IMO, thats correct.

Regards
Nikunj