Re: [PATCH v2 06/22] target/loongarch: Add main translation routines

[Song Gao]

Hi, Richard.

On 07/23/2021 07:50 AM, Richard Henderson wrote:
> On 7/20/21 11:53 PM, Song Gao wrote:
>> +/* General purpose registers moves. */
>> +void gen_load_gpr(TCGv t, int reg)
>> +{
>> +    if (reg == 0) {
>> +        tcg_gen_movi_tl(t, 0);
>> +    } else {
>> +        tcg_gen_mov_tl(t, cpu_gpr[reg]);
>> +    }
>> +}
> 
> Please have a look at
> 
> 20210709042608.883256-1-richard.henderson@linaro.org/">https://patchew.org/QEMU/20210709042608.883256-1-richard.henderson@linaro.org/
> 
> for a better way to handle the zero register.
> > 

OK, I'll look at it carefully.

>> +static inline void save_cpu_state(DisasContext *ctx, int do_save_pc)
>> +{
>> +    if (do_save_pc && ctx->base.pc_next != ctx->saved_pc) {
>> +        gen_save_pc(ctx->base.pc_next);
>> +        ctx->saved_pc = ctx->base.pc_next;
>> +    }
>> +    if (ctx->hflags != ctx->saved_hflags) {
>> +        tcg_gen_movi_i32(hflags, ctx->hflags);
>> +        ctx->saved_hflags = ctx->hflags;
>> +        switch (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
>> +        case LOONGARCH_HFLAG_BR:
>> +            break;
>> +        case LOONGARCH_HFLAG_BC:
>> +        case LOONGARCH_HFLAG_B:
>> +            tcg_gen_movi_tl(btarget, ctx->btarget);
>> +            break;
>> +        }
>> +    }
>> +}
> 
> Drop all the hflags handling.
> It's all copied from mips delay slot handling.
> 

OK.

>> +
>> +static inline void restore_cpu_state(CPULoongArchState *env, DisasContext 
>> *ctx)
>> +{
>> +    ctx->saved_hflags = ctx->hflags;
>> +    switch (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
>> +    case LOONGARCH_HFLAG_BR:
>> +        break;
>> +    case LOONGARCH_HFLAG_BC:
>> +    case LOONGARCH_HFLAG_B:
>> +        ctx->btarget = env->btarget;
>> +        break;
>> +    }
>> +}
> 
> Likewise.
> 
>> +static void gen_load_fpr32h(TCGv_i32 t, int reg)
>> +{
>> +    tcg_gen_extrh_i64_i32(t, fpu_f64[reg]);
>> +}
>> +
>> +static void gen_store_fpr32h(TCGv_i32 t, int reg)
>> +{
>> +    TCGv_i64 t64 = tcg_temp_new_i64();
>> +    tcg_gen_extu_i32_i64(t64, t);
>> +    tcg_gen_deposit_i64(fpu_f64[reg], fpu_f64[reg], t64, 32, 32);
>> +    tcg_temp_free_i64(t64);
>> +}
> 
> There is no general-purpose high-part fpr stuff.  There's only movgr2frh and 
> movfrh2gr, and you can simplify both if you drop the transition through 
> TCGv_i32.
> 
OK.

>> +void gen_op_addr_add(TCGv ret, TCGv arg0, TCGv arg1)
>> +{
>> +    tcg_gen_add_tl(ret, arg0, arg1);
>> +}
> 
> No point in this, since loongarch has no 32-bit address mode.
> 
OK.

>> +void gen_base_offset_addr(TCGv addr, int base, int offset)
>> +{
>> +    if (base == 0) {
>> +        tcg_gen_movi_tl(addr, offset);
>> +    } else if (offset == 0) {
>> +        gen_load_gpr(addr, base);
>> +    } else {
>> +        tcg_gen_movi_tl(addr, offset);
>> +        gen_op_addr_add(addr, cpu_gpr[base], addr);
>> +    }
>> +}
> 
> Using the interfaces I quote above from my riscv cleanup,
> this can be tidied to
> 
>     tcg_gen_addi_tl(addr, gpr_src(base), offset);
> 

'riscv cleanup' series at 
20210709042608.883256-1-richard.henderson@linaro.org/">https://patchew.org/QEMU/20210709042608.883256-1-richard.henderson@linaro.org/ 
, Right?


>> +static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
>> +{
>> +    return true;
>> +}
> 
> You must now use translate_use_goto_tb, which will not always return true.  
> You will see assertion failures otherwise.
> 

I see the patch already.

>> +static inline void clear_branch_hflags(DisasContext *ctx)
>> +{
>> +    ctx->hflags &= ~LOONGARCH_HFLAG_BMASK;
>> +    if (ctx->base.is_jmp == DISAS_NEXT) {
>> +        save_cpu_state(ctx, 0);
>> +    } else {
>> +        /*
>> +         * It is not safe to save ctx->hflags as hflags may be changed
>> +         * in execution time.
>> +         */
>> +        tcg_gen_andi_i32(hflags, hflags, ~LOONGARCH_HFLAG_BMASK);
>> +    }
>> +}
> 
> Not required.
> 
>> +static void gen_branch(DisasContext *ctx, int insn_bytes)
>> +{
>> +    if (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
>> +        int proc_hflags = ctx->hflags & LOONGARCH_HFLAG_BMASK;
>> +        /* Branches completion */
>> +        clear_branch_hflags(ctx);
>> +        ctx->base.is_jmp = DISAS_NORETURN;
>> +        switch (proc_hflags & LOONGARCH_HFLAG_BMASK) {
>> +        case LOONGARCH_HFLAG_B:
>> +            /* unconditional branch */
>> +            gen_goto_tb(ctx, 0, ctx->btarget);
>> +            break;
>> +        case LOONGARCH_HFLAG_BC:
>> +            /* Conditional branch */
>> +            {
>> +                TCGLabel *l1 = gen_new_label();
>> +
>> +                tcg_gen_brcondi_tl(TCG_COND_NE, bcond, 0, l1);
>> +                gen_goto_tb(ctx, 1, ctx->base.pc_next + insn_bytes);
>> +                gen_set_label(l1);
>> +                gen_goto_tb(ctx, 0, ctx->btarget);
>> +            }
>> +            break;
>> +        case LOONGARCH_HFLAG_BR:
>> +            /* unconditional branch to register */
>> +            tcg_gen_mov_tl(cpu_PC, btarget);
>> +            tcg_gen_lookup_and_goto_ptr();
>> +            break;
>> +        default:
>> +            fprintf(stderr, "unknown branch 0x%x\n", proc_hflags);
>> +            abort();
>> +        }
>> +    }
>> +}
> 
> Split this up into the various trans_* branch routines, without the setting 
> of HFLAG.
> 
>> +static void loongarch_tr_init_disas_context(DisasContextBase *dcbase,
>> +                                            CPUState *cs)
>> +{
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +    CPULoongArchState *env = cs->env_ptr;
>> +
>> +    ctx->page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
>> +    ctx->saved_pc = -1;
>> +    ctx->btarget = 0;
>> +    /* Restore state from the tb context.  */
>> +    ctx->hflags = (uint32_t)ctx->base.tb->flags;
>> +    restore_cpu_state(env, ctx);
>> +    ctx->mem_idx = LOONGARCH_HFLAG_UM;
> 
> This is not an mmu index.  You didn't notice the error because you're only 
> doing user-mode.
> 
> You're missing a check for page crossing.
> Generally, for fixed-width ISAs like this, we do
> 
>     /* Bound the number of insns to execute to those left on the page.  */
>     int bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
>     ctx->base.max_insns = MIN(ctx->base.max_insns, bound);
> 
> here in init_disas_context.
> 
>> +static void loongarch_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>> +{
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +
>> +    tcg_gen_insn_start(ctx->base.pc_next, ctx->hflags & 
>> LOONGARCH_HFLAG_BMASK,
>> +                       ctx->btarget);
> 
> No hflags/btarget stuff.  Drop TARGET_INSN_START_EXTRA_WORDS.
> 
>> +static bool loongarch_tr_breakpoint_check(DisasContextBase *dcbase,
>> +                                          CPUState *cs,
>> +                                          const CPUBreakpoint *bp)
>> +{
>> +    return true;
>> +}
> 
> Broken, but now handled generically, so remove it.
> 
> 
OK.

>> +static void loongarch_tr_translate_insn(DisasContextBase *dcbase, CPUState 
>> *cs)
>> +{
>> +    CPULoongArchState *env = cs->env_ptr;
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +    int insn_bytes = 4;
>> +
>> +    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
>> +
>> +    if (!decode(ctx, ctx->opcode)) {
>> +        fprintf(stderr, "Error: unkown opcode. 0x%lx: 0x%x\n",
>> +                ctx->base.pc_next, ctx->opcode);
> 
> No fprintfs.  Use qemu_log_mask with LOG_UNIMP or LOG_GUEST_ERROR.
> 
OK.
>> +    if (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
>> +        gen_branch(ctx, insn_bytes);
>> +    }
> 
> Drop this, as I mentioned above.
> 
OK.

>> +static void fpu_dump_state(CPULoongArchState *env, FILE * f, int flags)
>> +{
>> +    int i;
>> +    int is_fpu64 = 1;
>> +
>> +#define printfpr(fp)                                              \
>> +    do {                                                          \
>> +        if (is_fpu64)                                             \
>> +            qemu_fprintf(f, "w:%08x d:%016" PRIx64                \
>> +                        " fd:%13g fs:%13g psu: %13g\n",           \
>> +                        (fp)->w[FP_ENDIAN_IDX], (fp)->d,          \
>> +                        (double)(fp)->fd,                         \
>> +                        (double)(fp)->fs[FP_ENDIAN_IDX],          \
>> +                        (double)(fp)->fs[!FP_ENDIAN_IDX]);        \
>> +        else {                                                    \
>> +            fpr_t tmp;                                            \
>> +            tmp.w[FP_ENDIAN_IDX] = (fp)->w[FP_ENDIAN_IDX];        \
>> +            tmp.w[!FP_ENDIAN_IDX] = ((fp) + 1)->w[FP_ENDIAN_IDX]; \
>> +            qemu_fprintf(f, "w:%08x d:%016" PRIx64                \
>> +                        " fd:%13g fs:%13g psu:%13g\n",            \
>> +                        tmp.w[FP_ENDIAN_IDX], tmp.d,              \
>> +                        (double)tmp.fd,                           \
>> +                        (double)tmp.fs[FP_ENDIAN_IDX],            \
>> +                        (double)tmp.fs[!FP_ENDIAN_IDX]);          \
>> +        }                                                         \
>> +    } while (0)
> 
> This is broken.  You're performing an integer to fp conversion of something 
> that is already a floating-point value, not printing the floating-point value 
> itself.  It's broken in the mips code as well.
> 
> In addition, is_fpu64 is pointless for loongarch.
> 
Yes.
>> +void loongarch_tcg_init(void)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < 32; i++)
>> +        cpu_gpr[i] = tcg_global_mem_new(cpu_env,
>> +                                        offsetof(CPULoongArchState,
>> +                                                 active_tc.gpr[i]),
>> +                                        regnames[i]);
> 
> Missing braces.
> Do not create a temp for the zero register.
> 
>> +    bcond = tcg_global_mem_new(cpu_env,
>> +                               offsetof(CPULoongArchState, bcond), "bcond");
>> +    btarget = tcg_global_mem_new(cpu_env,
>> +                                 offsetof(CPULoongArchState, btarget),
>> +                                 "btarget");
>> +    hflags = tcg_global_mem_new_i32(cpu_env,
>> +                                    offsetof(CPULoongArchState, hflags),
>> +                                    "hflags");
> 
> Drop these.
OK.

Thanks for you kindly help.

Thanks
Song Gao.