Re: [Qemu-devel] [PATCH RFC v4 02/12] target/rx: TCG helper

[Yoshinori Sato]

On Fri, 22 Mar 2019 02:17:35 +0900,
Richard Henderson wrote:
> 
> On 3/20/19 7:16 AM, Yoshinori Sato wrote:
> > +void rx_cpu_unpack_psw(CPURXState *env, int all)
> > +{
> > +    if (env->psw_pm == 0) {
> > +        env->psw_ipl = extract32(env->psw, PSW_IPL, 4);
> > +        if (all) {
> > +            env->psw_pm = extract32(env->psw, PSW_PM, 1);
> > +        }
> > +        env->psw_u =  extract32(env->psw, PSW_U, 1);
> > +        env->psw_i =  extract32(env->psw, PSW_I, 1);
> > +    }
> > +    env->psw_o = extract32(env->psw, PSW_O, 1) << 31;
> > +    env->psw_s = extract32(env->psw, PSW_S, 1) << 31;
> > +    env->psw_z = extract32(env->psw, PSW_Z, 1) ? 0 : 1;
> > +    env->psw_c = extract32(env->psw, PSW_C, 1);
> > +}
> 
> I think this function should take the psw value to extract as an argument.
> Otherwise, if psw_pm == 1, you're leaving garbage values in env->psw, and I
> think that can be confusing.
> 
> In fact, since pack_psw does not read env->psw at all, I would remove that
> variable.

OK.

> > +uint32_t helper_mvfc(CPURXState *env, uint32_t cr)
> > +{
> > +    switch (cr) {
> > +    case 0:
> > +        return pack_psw(env);
> > +    case 2:
> > +        return env->psw_u ? env->regs[0] : env->usp;
> > +    case 3:
> > +        return env->fpsw;
> > +    case 8:
> > +        return env->bpsw;
> > +    case 9:
> > +        return env->bpc;
> > +    case 10:
> > +        return env->psw_u ? env->isp : env->regs[0];
> > +    case 11:
> > +        return env->fintv;
> > +    case 12:
> > +        return env->intb;
> > +    default:
> > +        g_assert_not_reached();
> > +        return -1;
> 
> This should only assert if it is caught in translate.c and rejected as an
> invalid instruction.  I don't see that happening though.

PC is set to trans_MVFC.
But it difficult. fixed.

> I think the best option is to move all of this into translate.c.  You have
> defined tcg temporaries for (almost) all of these, although many are currently
> unused. It's easy to handle any without tcg temporaries with
> 
>   tcg_gen_ld_i32(tmp, cpu_env, offsetof(CPURXState, field));
> 
> At which point your default case can simply return false to signal an illegal
> instruction.
> 
> I now see that PC is a control register number 1, not handled here, and that
> "MVFC PC, rN" should use ctx->pc.

OK.
mvfc and mvtc body move to translate.c
Since the wrong operand does not become an invalid instruction,
I only output the log.

> 
> > +void helper_mvtc(CPURXState *env, uint32_t cr, uint32_t val)
> > +{
> > +    switch (cr) {
> ...
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +}
> 
> Likewise.
> 
> > +void helper_unpack_psw(CPURXState *env)
> > +{
> > +    uint32_t prev_u;
> > +    prev_u = env->psw_u;
> > +    rx_cpu_unpack_psw(env, 1);
> > +    if (prev_u != env->psw_u) {
> > +        if (env->psw_u) {
> > +            env->isp = env->regs[0];
> > +            env->regs[0] = env->usp;
> > +        } else {
> > +            env->usp = env->regs[0];
> > +            env->regs[0] = env->isp;
> > +        }
> > +    }
> > +}
> 
> Again, I think this should take the full psw as an argument.
> 
> > +/* fp operations */
> > +static void update_fpsw(CPURXState *env, float32 ret, uintptr_t retaddr)
> > +{
> > +    int xcpt, cause, enable;
> > +
> > +    env->psw_z = (*((uint32_t *)&ret) == 0); \
> > +    env->psw_s = (*((uint32_t *)&ret) >= 0x80000000UL); \
> 
> (1) You do not need this casting, float32 == uint32_t,
> (2) The result is wrong for -0.0.
> 
>     env->psw_s = ret;               /* sign in bit 31 */
>     env->psw_z = ret & 0x7fffffff;  /* remove sign from -0.0 */

OK.

> > +    xcpt = get_float_exception_flags(&env->fp_status);
> > +
> > +    /* Clear the cause entries */
> > +    env->fpsw = deposit32(env->fpsw, FPSW_CAUSE, 5, 0);
> > +
> > +    if (unlikely(xcpt)) {
> > +        if (xcpt & float_flag_invalid) {
> > +            env->fpsw = deposit32(env->fpsw, FPSW_CAUSE_V, 1, 1);
> > +            env->fpsw = deposit32(env->fpsw, FPSW_FLAG_V, 1, 1);
> > +        }
> > +        if (xcpt & float_flag_divbyzero) {
> > +            env->fpsw = deposit32(env->fpsw, FPSW_CAUSE_Z, 1, 1);
> > +            env->fpsw = deposit32(env->fpsw, FPSW_FLAG_Z, 1, 1);
> > +        }
> > +        if (xcpt & float_flag_overflow) {
> > +            env->fpsw = deposit32(env->fpsw, FPSW_CAUSE_O, 1, 1);
> > +            env->fpsw = deposit32(env->fpsw, FPSW_FLAG_O, 1, 1);
> > +        }
> > +        if (xcpt & float_flag_underflow) {
> > +            env->fpsw = deposit32(env->fpsw, FPSW_CAUSE_U, 1, 1);
> > +            env->fpsw = deposit32(env->fpsw, FPSW_FLAG_U, 1, 1);
> > +        }
> > +        if (xcpt & float_flag_inexact) {
> > +            env->fpsw = deposit32(env->fpsw, FPSW_CAUSE_X, 1, 1);
> > +            env->fpsw = deposit32(env->fpsw, FPSW_FLAG_X, 1, 1);
> > +        }
> 
> What I don't see here is the "unimplemented processing" exception that you get
> for denormal inputs or outputs, and DN = 0.
> 
> There is support for this in softfloat, although it looks funny.
> You must always have
> 
>   set_flush_to_zero(1, &env->fp_status);
>   set_flush_inputs_to_zero(1, &env->fp_status);
> 
> Do this during reset, I think.
> 
> This enables within softfloat the float_flag_{input,output}_denormal bits.
> Here in update_fpsw you would do
> 
>     if ((xcpt & (float_flag_input_denormal
>                  | float_flag_output_denormal))
>         && !(env->fpsw & FPSW_DN)) {
>         env->fpsw = deposit32(env->fpsw, FPSW_CAUSE_E, 1, 1);
>     }
> 
> > +        /* Generate an exception if enabled */
> > +        cause = extract32(env->fpsw, FPSW_CAUSE, 5);
> > +        enable = extract32(env->fpsw, FPSW_ENABLE, 5);
> 
>     /* Cause E, unimplemented processing, cannot be disabled.  */
>     enable |= (1 << 5);
> 
> > +        if (cause & enable) {
> > +            raise_exception(env, 21, retaddr);
> > +        }
> > +    }
> > +}

OK.

> > +
> > +static void set_fpmode(CPURXState *env, uint32_t val)
> > +{
> > +    static const int roundmode[] = {
> > +        float_round_nearest_even,
> > +        float_round_to_zero,
> > +        float_round_up,
> > +        float_round_down,
> > +    };
> > +    env->fpsw = val & FPSW_MASK;
> > +    set_float_rounding_mode(roundmode[val & FPSW_RM_MASK],
> > +                            &env->fp_status);
> > +    set_flush_to_zero((val & FPSW_DN) != 0, &env->fp_status);
> 
> As discussed above, do not set_flush_to_zero here.

OK. Remove it.
Since the handling of fpsw was not correct, I am making some fix.

> > +#define FLOATOP(op, func)                                           \
> > +float32 helper_##op(CPURXState *env, float32 t0, float32 t1) \
> > +{ \
> > +    float32 ret; \
> > +    ret = func(t0, t1, &env->fp_status); \
> > +    update_fpsw(env, *(uint32_t *)&ret, GETPC());        \
> 
> No casting required.

OK.

> > +void helper_fcmp(CPURXState *env, float32 t0, float32 t1)
> > +{
> > +    int st;
> > +    st = float32_compare(t0, t1, &env->fp_status);
> > +    update_fpsw(env, 0, GETPC());
> > +    env->psw_z = env->psw_s = env->psw_o = 0;
> 
> env->psw_z = 1, because Z == !psw_z.

OK.
It seems that I forgot the changes last time.

> > +    switch (st) {
> > +    case float_relation_equal:
> > +        env->psw_z = 0;
> > +        break;
> > +    case float_relation_less:
> > +        env->psw_s = -1;
> > +        break;
> > +    case float_relation_unordered:
> > +        env->psw_o = 1 << 31;
> 
>    -1 works here too.

OK.

> 
> > +void helper_sstr(CPURXState *env, uint32_t sz)
> > +{
> > +    while (env->regs[3] != 0) {
> > +        switch (sz) {
> > +        case 0:
> > +            cpu_stb_data_ra(env, env->regs[1], env->regs[2], GETPC());
> > +            break;
> > +        case 1:
> > +            cpu_stw_data_ra(env, env->regs[1], env->regs[2], GETPC());
> > +            break;
> > +        case 2:
> > +            cpu_stl_data_ra(env, env->regs[1], env->regs[2], GETPC());
> > +            break;
> > +        }
> > +        env->regs[1] += (1 << sz);
> > +        env->regs[3]--;
> > +    }
> 
> This should probably be split into 3 functions, so that the switch does not
> have to be re-evaluated within the loop.

OK.
I made it a jump table.

> > +static void rx_search(uint32_t mode, int sz, CPURXState *env)
> > +{
> > +    uint32_t tmp;
> > +
> > +    while (env->regs[3] != 0) {
> > +        tmp = ld[sz](env, env->regs[1], env->pc);
> > +        env->regs[1] += 1 << (mode % 4);
> 
> This increment doesn't look right for SWHILE.
> 
> > +        env->regs[3]--;
> > +        if ((mode == OP_SWHILE && tmp != env->regs[2]) ||
> > +            (mode == OP_SUNTIL && tmp == env->regs[2])) {
> > +            break;
> > +        }
> > +    }
> > +    env->psw_z = (mode == OP_SUNTIL) ?
> > +        (tmp - env->regs[2]) : env->regs[3];
> > +    env->psw_c = (tmp <= env->regs[2]);
> > +}
> 
> I think this whole function would be clearer split between SWHILE and SUNTIL.
> You're missing the initial R3 == 0 check (if executed with R3 == 0, no effect
> on registers or flags).

OK.
It split helper_suntil and helper_swhile.

> 
> > +/* accumlator operations */
> > +void helper_rmpa(CPURXState *env, uint32_t sz)
> > +{
> > +    uint64_t result_l, prev;
> > +    int32_t result_h;
> > +    int64_t tmp0, tmp1;
> > +
> > +    if (env->regs[3] == 0) {
> > +        return;
> > +    }
> > +    result_l = env->regs[5];
> > +    result_l <<= 32;
> > +    result_l |= env->regs[4];
> > +    result_h = env->regs[6];
> > +    env->psw_o = 0;
> > +
> > +    while (env->regs[3] != 0) {
> > +        tmp0 = ld[sz](env, env->regs[1], env->pc);
> > +        tmp1 = ld[sz](env, env->regs[2], env->pc);
> 
> You need to use signed load functions here.

OK.

> > +void helper_satr(CPURXState *env)
> > +{
> > +    if (env->psw_o >> 31) {
> > +        if ((int)env->psw_s < 0) {
> > +            env->regs[4] = 0x00000000;
> > +            env->regs[5] = 0x7fffffff;
> > +            env->regs[6] = 0xffffffff;
> > +        } else {
> > +            env->regs[4] = 0xffffffff;
> > +            env->regs[5] = 0x80000000;
> > +            env->regs[6] = 0x00000000;
> 
> These constants are in the wrong order.

OK.
It is my mistake.

> > +/* div */
> > +uint32_t helper_div(CPURXState *env, uint32_t num, uint32_t den)
> > +{
> > +    uint32_t ret = num;
> > +    if (den != 0) {
> > +        ret = (int32_t)num / (int32_t)den;
> > +    }
> > +    env->psw_o = ((num == INT_MIN && den == -1) || den == 0) << 31;
> 
> Because of x86 host, which will raise SIGFPE for INT_MIN/-1, this needs to be
> 
>     if (den == 0 || (num == INT_MIN && den == -1)) {
>         ret = num;  /* undefined */
>         env->psw_o = -1;
>     } else {
>         ret = (int32_t)num / (int32_t)den;
>         env->psw_o = 0;
>     }
> 
> 
> r~
> 

OK.
I checked the overflow before divide.

-- 
Yosinori Sato