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
