Re: [Qemu-devel] [PATCH v3 19/40] target/mips: Add emulation of nanoMIPS branch instructions

[Richard Henderson]

On 07/19/2018 05:54 AM, Stefan Markovic wrote:
> +/* Immediate Value Compact Branches */
> +static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
> +                                   int rt, int32_t imm, int32_t offset)
> +{
> +    int bcond_compute = 0;
> +    TCGv t0 = tcg_temp_new();
> +    TCGv t1 = tcg_temp_new();
> +
> +    if (ctx->hflags & MIPS_HFLAG_BMASK) {
> +#ifdef MIPS_DEBUG_DISAS
> +        LOG_DISAS("Branch in delay / forbidden slot at PC 0x" TARGET_FMT_lx
> +                  "\n", ctx->base.pc_next);
> +#endif
> +        generate_exception_end(ctx, EXCP_RI);
> +        goto out;
> +    }
> +
> +    gen_load_gpr(t0, rt);
> +    tcg_gen_movi_tl(t1, imm);
> +    ctx->btarget = addr_add(ctx, ctx->base.pc_next + 4, offset);
> +
> +    /* Load needed operands and calculate btarget */
> +    switch (opc) {
> +    case NM_BEQIC:
> +        if (rt == 0 && imm == 0) {
> +            /* Unconditional branch */
> +        } else if (rt == 0 && imm != 0) {
> +            /* Treat as NOP */
> +            goto out;

Surely this misses out on tracking the forbidden slot.
Given how infrequently these cases happen, is it really
worth special casing these at all?

Etc, througout this patch.

> +    if (bcond_compute == 0) {
> +        /* Uncoditional compact branch */
> +        ctx->hflags |= MIPS_HFLAG_B;
> +        /* Generating branch here as compact branches don't have delay slot 
> */
> +        gen_branch(ctx, 4);
> +    } else {
> +        /* Conditional compact branch */
> +        TCGLabel *fs = gen_new_label();
> +        save_cpu_state(ctx, 0);
> +
> +        switch (opc) {
> +        case NM_BEQIC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_EQ), t0, t1, fs);
> +            break;
> +        case NM_BBEQZC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_EQ), t0, t1, fs);
> +            break;
> +        case NM_BNEIC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_NE), t0, t1, fs);
> +            break;
> +        case NM_BBNEZC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_NE), t0, t1, fs);
> +            break;
> +        case NM_BGEIC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_GE), t0, t1, fs);
> +            break;
> +        case NM_BLTIC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_LT), t0, t1, fs);
> +            break;
> +        case NM_BGEIUC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_GEU), t0, t1, fs);
> +            break;
> +        case NM_BLTIUC:
> +            tcg_gen_brcond_tl(tcg_invert_cond(TCG_COND_LTU), t0, t1, fs);
> +            break;
> +        }

I think you should, in the previous switch, store these conditions into a
variable.  Then squish this switch out and just have one brcond.


r~