From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=51073 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OKbfK-00042O-3d
	for qemu-devel@nongnu.org; Fri, 04 Jun 2010 14:30:47 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <rth@twiddle.net>) id 1OKbfI-0003Qy-M1
	for qemu-devel@nongnu.org; Fri, 04 Jun 2010 14:30:45 -0400
Received: from are.twiddle.net ([75.149.56.221]:60538)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <rth@twiddle.net>) id 1OKbfH-0003Qj-VS
	for qemu-devel@nongnu.org; Fri, 04 Jun 2010 14:30:44 -0400
Message-ID: <4C09464E.3040102@twiddle.net>
Date: Fri, 04 Jun 2010 11:30:38 -0700
From: Richard Henderson <rth@twiddle.net>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 06/10] target-mips: add microMIPS ASE support
References: <1274717984-25887-1-git-send-email-froydnj@codesourcery.com>
	<1274717984-25887-7-git-send-email-froydnj@codesourcery.com>
In-Reply-To: <1274717984-25887-7-git-send-email-froydnj@codesourcery.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Nathan Froyd <froydnj@codesourcery.com>
Cc: qemu-devel@nongnu.org, aurelien@aurel32.net

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~