Re: [Qemu-devel] [PATCH for-2.5 26/30] m68k: add mull/divl

[Richard Henderson]

On 08/09/2015 01:13 PM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  target-m68k/cpu.h       |   3 +
>  target-m68k/helper.h    |   6 ++
>  target-m68k/op_helper.c | 143 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-m68k/qregs.def   |   1 +
>  target-m68k/translate.c |  65 ++++++++++++++++++----
>  5 files changed, 208 insertions(+), 10 deletions(-)
> 
> diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
> index 6d1a140..a261680 100644
> --- a/target-m68k/cpu.h
> +++ b/target-m68k/cpu.h
> @@ -90,6 +90,9 @@ typedef struct CPUM68KState {
>      uint32_t div1;
>      uint32_t div2;
>  
> +    /* Upper 32 bits of a 64bit operand for quad MUL/DIV.  */
> +    uint32_t quadh;

This is a poor choice, IMO.

> +void HELPER(divu64)(CPUM68KState *env)
> +{
> +    uint32_t num;
> +    uint32_t den;
> +    uint64_t quot;
> +    uint32_t rem;
> +    uint32_t flags;
> +    uint64_t quad;
> +
> +    num = env->div1;
> +    den = env->div2;
> +    /* ??? This needs to make sure the throwing location is accurate.  */
> +    if (den == 0) {
> +        raise_exception(env, EXCP_DIV0);
> +    }
> +    quad = num | ((uint64_t)env->quadh << 32);

Pass in the numerator as a 64-bit argument, and avoid the quadh input.

> +    quot = quad / den;
> +    rem = quad % den;
> +    if (quot > 0xffffffffULL) {
> +        flags = (env->cc_dest & ~CCF_C) | CCF_V;
> +    } else {
> +        flags = 0;
> +        if (quot == 0) {
> +            flags |= CCF_Z;
> +        } else if ((int32_t)quot < 0) {
> +            flags |= CCF_N;
> +        }
> +        env->div1 = quot;
> +        env->quadh = rem;

Return a 64-bit result with the quot/rem packed into the high/low.

> +void HELPER(divs64)(CPUM68KState *env)

Similarly.

> +    if ((quot & 0xffffffff80000000ULL) &&
> +        (quot & 0xffffffff80000000ULL) != 0xffffffff80000000ULL) {

Possibly more concisely written as

  !((quot >> 31) == 0 || (quot >> 31) == -1)

> +uint32_t HELPER(mulu32_cc)(CPUM68KState *env, uint32_t op1, uint32_t op2)
> +uint32_t HELPER(muls32_cc)(CPUM68KState *env, uint32_t op1, uint32_t op2)
> +uint32_t HELPER(mulu64)(CPUM68KState *env, uint32_t op1, uint32_t op2)
> +uint32_t HELPER(muls64)(CPUM68KState *env, uint32_t op1, uint32_t op2)

It's much easier to do all of the multiplication inline now.  I suppose the
opcodes involved didn't exist when this patch was written.  See below.

It's probably better to split this patch in two as well, one part for division
and the other for multiplication.

> @@ -1122,8 +1122,27 @@ DISAS_INSN(divl)
>      uint16_t ext;
>  
>      ext = read_im16(env, s);
> -    if (ext & 0x87f8) {
> -        gen_exception(s, s->pc - 4, EXCP_UNSUPPORTED);
> +    if (ext & 0x400) {
> +        if (!m68k_feature(s->env, M68K_FEATURE_QUAD_MULDIV)) {
> +            gen_exception(s, s->pc - 4, EXCP_UNSUPPORTED);
> +            return;
> +        }
> +        num = DREG(ext, 12);
> +        reg = DREG(ext, 0);
> +        tcg_gen_mov_i32(QREG_DIV1, num);
> +        tcg_gen_mov_i32(QREG_QUADH, reg);

This then becomes

           TCGv_i64 t64 = tcg_temp_new_i64();
           tcg_gen_concat_i32_i64(t64, num, reg);

> +        SRC_EA(env, den, OS_LONG, 0, NULL);
> +        tcg_gen_mov_i32(QREG_DIV2, den);
> +        if (ext & 0x0800) {
> +            gen_helper_divs64(cpu_env);
> +        } else {
> +            gen_helper_divu64(cpu_env);

               gen_helper_divu64(t64, cpu_env, t64, den);

> +        }

           TCGv_i32 q = tcg_temp_new();
           TCGv_i32 r = tcg_temp_new();
           tcg_gen_extr_i64_i32(q, r, t64);
           tcg_temp_free_i64(t64);

> +        tcg_gen_mov_i32(num, QREG_DIV1);
> +        if (!TCGV_EQUAL(num, reg)) {
> +            tcg_gen_mov_i32(reg, QREG_QUADH);
> +        }

Depending on how common this is, or isn't, it's probably cleaner to trust the
tcg optimizer to delete the dead code:

           /* If Dq and Dr are the same, the quotient is returned.
              therefore we set Dq last.  */
           tcg_gen_mov_i32(reg, r);
           tcg_gen_mov_i32(num, q);

> @@ -1887,21 +1908,45 @@ DISAS_INSN(mull)
>      TCGv reg;
>      TCGv src1;
>      TCGv dest;
> +    TCGv regh;
>  
>      /* The upper 32 bits of the product are discarded, so
>         muls.l and mulu.l are functionally equivalent.  */
>      ext = read_im16(env, s);
> -    if (ext & 0x87ff) {
> -        gen_exception(s, s->pc - 4, EXCP_UNSUPPORTED);
> +    if (ext & 0x400) {
> +        if (!m68k_feature(s->env, M68K_FEATURE_QUAD_MULDIV)) {
> +            gen_exception(s, s->pc - 4, EXCP_UNSUPPORTED);
> +            return;
> +        }
> +        reg = DREG(ext, 12);
> +        regh = DREG(ext, 0);
> +        SRC_EA(env, src1, OS_LONG, 0, NULL);
> +        dest = tcg_temp_new();
> +        if (ext & 0x800) {
> +            gen_helper_muls64(dest, cpu_env, src1, reg);

        tcg_gen_muls2_i32(reg, dest, reg, src1);
        set_cc_op(s, CC_OP_MULQ);
        tcg_gen_mov_i32(QREG_CC_DEST, dest);
        tcg_gen_mov_i32(QREG_CC_SRC, reg);
        tcg_gen_mov_i32(regh, dest);

where MULQ sets N = (CC_DEST < 0) and Z = ((CC_DEST | CC_SRC) == 0).

> +        } else {
> +            gen_helper_mulu64(dest, cpu_env, src1, reg);

Similarly with tcg_gen_mulu2_i32.

> -    tcg_gen_mul_i32(dest, src1, reg);
> -    tcg_gen_mov_i32(reg, dest);
> -    /* Unlike m68k, coldfire always clears the overflow bit.  */
> +    if (m68k_feature(s->env, M68K_FEATURE_M68000)) {
> +        if (ext & 0x800) {
> +            gen_helper_muls32_cc(dest, cpu_env, src1, reg);

        tcg_gen_muls2_i32(reg, QREG_CC_DEST, reg, src1);
        set_cc_op(s, CC_OP_MULQ_V);
        tcg_gen_mov_i32(QREG_CC_SRC, reg);

where MULQ_V is like MULQ, except that it also sets
V = (DEST != (SRC >> 31)).

> +        } else {
> +            gen_helper_mulu32_cc(dest, cpu_env, src1, reg);

        tcg_gen_mulu2_i32(reg, QREG_CC_DEST, reg, src1);
        set_cc_op(s, CC_OP_MULQ);
        tcg_gen_mov_i32(QREG_CC_SRC, reg);


r~