Re: [Qemu-devel] [PATCH v2 2/2] target-m68k: add 680x0 divu/divs variants

[Richard Henderson]

On 10/28/2016 04:01 PM, Laurent Vivier wrote:
> -void HELPER(divu)(CPUM68KState *env, uint32_t word)
> +uint64_t HELPER(divu)(CPUM68KState *env, uint32_t num, uint32_t den,
> +                      uint32_t word)
>  {
> -    uint32_t num;
> -    uint32_t den;
>      uint32_t quot;
>      uint32_t rem;
>
> -    num = env->div1;
> -    den = env->div2;
> -    /* ??? This needs to make sure the throwing location is accurate.  */
>      if (den == 0) {
> -        raise_exception(env, EXCP_DIV0);
> +        raise_exception_ra(env, EXCP_DIV0, GETPC());
>      }
>      quot = num / den;
>      rem = num % den;
>
>      env->cc_v = (word && quot > 0xffff ? -1 : 0);

It would be better to have separate routines for word and !word.

> -    env->cc_z = quot;
> -    env->cc_n = quot;
> +    /* real 68040 keep Z and N on overflow,
> +     * whereas documentation says "undefined"
> +     */
> +    if (!env->cc_v) {
> +        env->cc_z = quot;
> +        env->cc_n = quot;
> +    }
>      env->cc_c = 0;
>
> -    env->div1 = quot;
> -    env->div2 = rem;
> +    return quot | ((uint64_t)rem << 32);

You ought to pack the data into 32 bits here,

> +    /* result rem.w:quot.w */
> +
> +    quot = tcg_temp_new_i32();
> +    rem = tcg_temp_new_i32();
> +    tcg_gen_extr_i64_i32(quot, rem, t64);
> +    tcg_temp_free_i64(t64);
> +    tcg_gen_deposit_i32(quot, quot, rem, 16, 16);
> +    tcg_temp_free_i32(rem);

instead of here.

> +
> +    /* on overflow, operands are unaffected,
> +     * Z and N flags are undefined, C is always 0
> +     */
> +
> +    tcg_gen_movcond_i32(TCG_COND_EQ, DREG(insn, 9),
> +                        QREG_CC_V, QREG_CC_C /* zero */,
> +                        quot, DREG(insn, 9));
> +    tcg_temp_free_i32(quot);
> +

However, given how overflow conditionally sets the register(s), I also wonder 
if it wouldn't be cleaner to pass the output register numbers to the helper.

E.g.

    t = tcg_const_i32(REG(insn, 9));
    gen_helper_divsw(cpu_env, t, src);
    tcg_temp_free_i32(t);

HELPER_3(divsw, env, int, s32)

void HELPER(divsw)(CPUM68KState *env, int destr, int32_t den)
{
    int32_t num = env->dregs[destr];
    int32_t quot, rem;

    if (den == 0) {
        ....
    }
    quot = num / den;
    rem = num % den;

    env->cc_c = 0;  /* always cleared, even if overflow */
    if (quot != (int16_t)quot) {
        env->cc_v = -1;
        /* nothing else is modified */
        return;
    }

    env->dregs[destr] = deposit32(quot, 16, 16, rem);
    env->cc_n = quot;
    env->cc_z = quot;
    env->cc_v = 0;
}

> +        /* divs.l <EA>, Dq        32/32 -> 32q     */
> +        /* divsl.l <EA>, Dr:Dq    32/32 -> 32r:32q */
> +        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(ext, 0),
> +                            QREG_CC_V, QREG_CC_C /* zero */,
> +                            rem, DREG(ext, 0)); /* rem */
> +        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(ext, 12),
> +                            QREG_CC_V, QREG_CC_C /* zero */,
> +                            quot, DREG(ext, 12)); /* quot */

This doubly applies to the 64-bit output variants.


r~