Re: [Qemu-devel] [PATCH 13/21] target-mips: add Compact Branches

[Aurelien Jarno]

On Tue, Jun 03, 2014 at 10:29:45AM +0100, Leon Alrae wrote:
> On 02/06/14 20:16, Aurelien Jarno wrote:
> >> -    case OPC_DADDI:
> >> +    case OPC_DADDI: /* OPC_BNVC, OPC_BNEZALC, OPC_BNEC */
> >> +        if (ctx->insn_flags & ISA_MIPS32R6) {
> >> +            /* OPC_BNVC, OPC_BNEZALC, OPC_BNEC */
> >> +            gen_compute_compact_branch(ctx, op, rs, rt, imm << 2);
> >> +        } else {
> >> +            /* OPC_DADDI */
> >> +            check_insn(ctx, ISA_MIPS3);
> >> +            check_mips_64(ctx);
> >> +            gen_arith_imm(ctx, op, rt, rs, imm);
> >> +        }
> >> +        break;
> >>      case OPC_DADDIU:
> >>          check_insn(ctx, ISA_MIPS3);
> >>          check_mips_64(ctx);
> >>          gen_arith_imm(ctx, op, rt, rs, imm);
> >>          break;
> >> +#else
> >> +    case OPC_BNVC: /* OPC_BNEZALC, OPC_BNEC */
> >> +        if (ctx->insn_flags & ISA_MIPS32R6) {
> >> +            gen_compute_compact_branch(ctx, op, rs, rt, imm << 2);
> >> +        } else {
> >> +            MIPS_INVAL("major opcode");
> >> +            generate_exception(ctx, EXCP_RI);
> >> +        }
> >> +        break;
> > 
> > Why is that introduced only in the MIPS32 case, but not in the MIPS64
> > case.
> Do you mean BNVC, BNEZALC and BNEC instructions? They are introduced in
> MIPS64 as well, you'll find them in OPC_DADDI case. OPC_DADDI was
> removed in R6 and its encoding has been reused for those instructions.

Oh correct, I missed that part. It might be a good idea to add a small
comment about that after the #else.

> There's quite a number of good findings. Thanks for the comments and
> suggestions.

I will continue to review the remaining patches as time permit, probably
tomorrow.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net