From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:39956)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1XW4Kf-0006qj-62
	for qemu-devel@nongnu.org; Mon, 22 Sep 2014 10:11:20 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1XW4Jw-0001nk-Jb
	for qemu-devel@nongnu.org; Mon, 22 Sep 2014 10:11:13 -0400
Received: from mailapp01.imgtec.com ([195.59.15.196]:45381)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Leon.Alrae@imgtec.com>) id 1XW4Jw-0001jK-Eh
	for qemu-devel@nongnu.org; Mon, 22 Sep 2014 10:10:28 -0400
Message-ID: <54202A35.9040405@imgtec.com>
Date: Mon, 22 Sep 2014 14:55:01 +0100
From: Leon Alrae <leon.alrae@imgtec.com>
MIME-Version: 1.0
References: <1404232985-63404-1-git-send-email-yongbok.kim@imgtec.com>
	<22483EC52335AD41841BD2BC97D39CFC36FB7687@hhmail02.hh.imgtec.org>
In-Reply-To: <22483EC52335AD41841BD2BC97D39CFC36FB7687@hhmail02.hh.imgtec.org>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2] target-mips: fix broken MIPS16 and
	microMIPS
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/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: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Yongbok Kim <Yongbok.Kim@imgtec.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: Cristian Cuna <Cristian.Cuna@imgtec.com>, "aurelien@aurel32.net" <aurelien@aurel32.net>

Hi Yongbok,

For me this patch looks fine, just minor nitpicks:

>      if (blink > 0) {
>          int post_delay = insn_bytes;
>          int lowbit = !!(ctx->hflags & MIPS_HFLAG_M16);
>  
> -        if (opc != OPC_JALRC)
> -            post_delay += ((ctx->hflags & MIPS_HFLAG_BDS16) ? 2 : 4);
> -
> +        post_delay += delayslot_size;

Wouldn't it be better to remove this line add delayslot_size during
post_delay initialization?

> +    if (ctx->hflags & MIPS_HFLAG_BDS_STRICT) {
> +        switch (op & 0x7) { /* MSB-3..MSB-5 */
> +        case 0:
> +        /* POOL31A, POOL32B, POOL32I, POOL32C */

POOL32A :)

>      case OPC_J ... OPC_JAL: /* Jump */
>           offset = (int32_t)(ctx->opcode & 0x3FFFFFF) << 2;
> -         gen_compute_branch(ctx, op, 4, rs, rt, offset);
> +         gen_compute_branch(ctx, op, 4, rs, rt, offset, 4);
>           break;
>      case OPC_BEQ ... OPC_BGTZ: /* Branch */
>      case OPC_BEQL ... OPC_BGTZL:
> -         gen_compute_branch(ctx, op, 4, rs, rt, imm << 2);
> +            gen_compute_branch(ctx, op, 4, rs, rt, imm << 2, 4);

Indentation issue?

> @@ -15719,6 +15648,13 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>              ctx.bstate = BS_STOP;
>              break;
>          }
> +        if (ctx.hflags & MIPS_HFLAG_BMASK) {
> +            if (!(ctx.hflags & MIPS_HFLAG_BDS16) &&
> +                    !(ctx.hflags & MIPS_HFLAG_BDS32)) {

IMHO it would look nicer if you made this condition shorter by ORing BDS
hflags.

Feel free to add:

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

Regards,
Leon