Re: [Qemu-devel] [PATCH 06/10] target-mips: add microMIPS ASE support

2010-06-04 Thread Richard Henderson 
On 05/24/2010 09:19 AM, Nathan Froyd wrote:
 +int (*ldfun)(target_ulong);
 +
 +switch (mem_idx)
 +{
 +case 0: ldfun = ldl_kernel; break;
 +case 1: ldfun = ldl_super; break;
 +default:
 +case 2: ldfun = ldl_user; break;
 +}

This *should* now be a compile error.  The return type should
now be uint32_t, not int.

 +env-active_tc.gpr[multiple_regs[i]] = ldfun(addr);
...
 +env-active_tc.gpr[31] = ldfun(addr);

Which means these will need explicit sign-extensions.

 +void (*stfun)(target_ulong, int);

Similarly uint32_t, though no other changes should be required.

 @@ -2535,26 +2555,29 @@ static void gen_compute_branch (DisasContext *ctx, 
 uint32_t opc,
  case OPC_JALX:
  ctx-hflags |= MIPS_HFLAG_BX;
  /* Fallthrough */
 +case OPC_JALS:
  case OPC_JAL:
  blink = 31;
  ctx-hflags |= MIPS_HFLAG_B;
 -ctx-hflags |= (ctx-hflags  MIPS_HFLAG_M16
 +ctx-hflags |= (opc == OPC_JALS
  ? MIPS_HFLAG_BDS16
  : MIPS_HFLAG_BDS32);

Changed semantics here?  You're no longer testing M16 bit.
Or is that later handled by switching mips16 to JALS too?

 @@ -8678,7 +8709,7 @@ static int decode_mips16_opc (CPUState *env, 
 DisasContext *ctx,
  int ra = (ctx-opcode  5)  0x1;
  
  if (link) {
 -op = nd ? OPC_JALRC : OPC_JALR;
 +op = nd ? OPC_JALRC : OPC_JALRS;

Here's one conversion of mips16 to the new S opcodes, but
this is the only one.  It *seems* like there should be more.
And if so, perhaps this patch should be broken into two, where
you introduce the new opcodes and hflags changes and apply them
as-needed to the mips16 code.  Thus one can verify that the
semantics for mips16 are the same before and after.

... Unless there's some micromips dependency I'm not seeing?

 +if (base == 0) {
 +tcg_gen_movi_tl(t0, 0);
 +} else {
 +gen_load_gpr(t0, base);
 +}

gen_load_gpr already takes care of R0.

 +#if 0
 +case 0x01:
 +switch (minor) {
 +case MFHI_ACC:
 +gen_HILO(ctx, OPC_MFHI, rs);

New if 0 code?


r~

Re: [Qemu-devel] [PATCH 06/10] target-mips: add microMIPS ASE support

2010-06-04 Thread Nathan Froyd 
On Fri, Jun 04, 2010 at 11:30:38AM -0700, Richard Henderson wrote:
 On 05/24/2010 09:19 AM, Nathan Froyd wrote:
  +int (*ldfun)(target_ulong);
  +
  +switch (mem_idx)
  +{
  +case 0: ldfun = ldl_kernel; break;
  +case 1: ldfun = ldl_super; break;
  +default:
  +case 2: ldfun = ldl_user; break;
  +}
 
 This *should* now be a compile error.  The return type should
 now be uint32_t, not int.

Will fix, thanks for pointing that out.

  @@ -2535,26 +2555,29 @@ static void gen_compute_branch (DisasContext *ctx, 
  uint32_t opc,
   case OPC_JALX:
   ctx-hflags |= MIPS_HFLAG_BX;
   /* Fallthrough */
  +case OPC_JALS:
   case OPC_JAL:
   blink = 31;
   ctx-hflags |= MIPS_HFLAG_B;
  -ctx-hflags |= (ctx-hflags  MIPS_HFLAG_M16
  +ctx-hflags |= (opc == OPC_JALS
   ? MIPS_HFLAG_BDS16
   : MIPS_HFLAG_BDS32);
 
 Changed semantics here?  You're no longer testing M16 bit.
 Or is that later handled by switching mips16 to JALS too?

It ought to be handled by switching mips16 to use JALS.  I see that I
didn't do that.

 And if so, perhaps this patch should be broken into two, where
 you introduce the new opcodes and hflags changes and apply them
 as-needed to the mips16 code.  Thus one can verify that the
 semantics for mips16 are the same before and after.

I don't think there are any hflags changes, but the point is
well-taken.  I'll do that.

  +if (base == 0) {
  +tcg_gen_movi_tl(t0, 0);
  +} else {
  +gen_load_gpr(t0, base);
  +}
 
 gen_load_gpr already takes care of R0.

Will fix.

  +#if 0
  +case 0x01:
  +switch (minor) {
  +case MFHI_ACC:
  +gen_HILO(ctx, OPC_MFHI, rs);
 
 New if 0 code?

Yup, will delete.

-Nathan