From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:47911)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jcmvbkbc@gmail.com>) id 1ScJHi-0005Ah-RH
	for qemu-devel@nongnu.org; Wed, 06 Jun 2012 12:40:40 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jcmvbkbc@gmail.com>) id 1ScJHZ-00051s-Cd
	for qemu-devel@nongnu.org; Wed, 06 Jun 2012 12:40:38 -0400
Received: from mail-gh0-f173.google.com ([209.85.160.173]:63280)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jcmvbkbc@gmail.com>) id 1ScJHZ-00051O-53
	for qemu-devel@nongnu.org; Wed, 06 Jun 2012 12:40:29 -0400
Received: by ghrr14 with SMTP id r14so6075342ghr.4
	for <qemu-devel@nongnu.org>; Wed, 06 Jun 2012 09:40:26 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <1338985632-29597-9-git-send-email-proljc@gmail.com>
References: <1338985632-29597-1-git-send-email-proljc@gmail.com>
	<1338985632-29597-9-git-send-email-proljc@gmail.com>
Date: Wed, 6 Jun 2012 20:40:26 +0400
Message-ID: <CAMo8BfL5uZsbj76WFuCAh5ko3Fx3fvrh4Uc9TLyb0vcm1veCUg@mail.gmail.com>
From: Max Filippov <jcmvbkbc@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH v3 08/16] target-or32: Add translation
	routines
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: Jia Liu <proljc@gmail.com>
Cc: qemu-devel@nongnu.org

Hi Jia,

more comments on remaining issues visible with unaided eye.

On Wed, Jun 6, 2012 at 4:27 PM, Jia Liu <proljc@gmail.com> wrote:
> Add OpenRISC translation routines.
>
> Signed-off-by: Jia Liu <proljc@gmail.com>
> ---

[...]

> + =A0 =A0case 0x0009:
> + =A0 =A0 =A0 =A0switch (op1) {
> + =A0 =A0 =A0 =A0case 0x03: =A0 /*l.div*/
> + =A0 =A0 =A0 =A0 =A0 =A0LOG_DIS("l.div r%d, r%d, r%d\n", rd, ra, rb);
> + =A0 =A0 =A0 =A0 =A0 =A0{
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv_i32 sr_ove;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int lab =3D gen_new_label();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sr_ove =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (rb =3D=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_brcondi_tl(TCG_COND_NE, =
sr_ove, SR_OVE, lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_exception(dc, EXCP_RANGE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_set_label(lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ra =3D=3D 0xffffffff && rb =
=3D=3D 0x80000000) {

Cannot do that: ra and rb are register numbers, not the values
contained in these registers.
Hence you need to generate code that will check these combinations of
register values.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_brcondi_tl(TCG_C=
OND_NE, sr_ove, SR_OVE, lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_exception(dc, EXCP_R=
ANGE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_set_label(lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_div_tl(cpu_R[rd]=
, cpu_R[ra], cpu_R[rb]);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(sr_ove);
> + =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0 =A0 =A0default:
> + =A0 =A0 =A0 =A0 =A0 =A0gen_illegal_exception(dc);
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0case 0x000a:
> + =A0 =A0 =A0 =A0switch (op1) {
> + =A0 =A0 =A0 =A0case 0x03: =A0 /*l.divu*/
> + =A0 =A0 =A0 =A0 =A0 =A0LOG_DIS("l.divu r%d, r%d, r%d\n", rd, ra, rb);
> + =A0 =A0 =A0 =A0 =A0 =A0if (rb =3D=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv_i32 sr_ove;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int lab =3D gen_new_label();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sr_ove =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, =
SR_OVE, lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_exception(dc, EXCP_RANGE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_set_label(lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(sr_ove);
> + =A0 =A0 =A0 =A0 =A0 =A0} else if (rb !=3D 0) {

'if (rb !=3D 0)' and the following 'else' block are redundant here.

I feel that I repeatedly fail to explain what's wrong with these div/divu
implementations; could you please add testcases for l.div and l.divu
that divide by the register other than r0 that contains 0 value?

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_divu_tl(cpu_R[rd], cpu_R[ra], cp=
u_R[rb]);
> + =A0 =A0 =A0 =A0 =A0 =A0} else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> + =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0 =A0 =A0default:
> + =A0 =A0 =A0 =A0 =A0 =A0gen_illegal_exception(dc);
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0case 0x000b:
> + =A0 =A0 =A0 =A0switch (op1) {
> + =A0 =A0 =A0 =A0case 0x03: =A0 /*l.mulu*/
> + =A0 =A0 =A0 =A0 =A0 =A0LOG_DIS("l.mulu r%d, r%d, r%d\n", rd, ra, rb);
> + =A0 =A0 =A0 =A0 =A0 =A0if (rb !=3D 0 && ra !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv result =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv high =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv tra =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv trb =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TCGv_i32 sr_ove =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int lab =3D gen_new_label();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int lab2 =3D gen_new_label();
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_extu_i32_i64(tra, cpu_R[ra]);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_extu_i32_i64(trb, cpu_R[rb]);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_mul_tl(result, cpu_R[ra], cpu_R[=
rb]);

You've calculated tra and trb but haven't uses them here.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_shri_tl(high, result, (sizeof(ta=
rget_ulong) * 8));

You probably meant result and high to be _i64.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_brcondi_tl(TCG_COND_EQ, high, 0x=
0, lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_CY);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ori_tl(cpu_sr, cpu_sr, SR_OV);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, =
SR_OVE, lab2);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_exception(dc, EXCP_RANGE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_set_label(lab);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gen_set_label(lab2);

No need to set two labels at one point.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_tl(cpu_R[rd], result);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(result);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(high);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(sr_ove);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(tra);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(trb);
> + =A0 =A0 =A0 =A0 =A0 =A0} else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_movi_tl(cpu_R[rd], 0);
> + =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0 =A0 =A0default:
> + =A0 =A0 =A0 =A0 =A0 =A0gen_illegal_exception(dc);
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0break;
> +

[...]

> + =A0 =A0case 0x13: =A0 =A0/*l.maci*/
> + =A0 =A0 =A0 =A0LOG_DIS("l.maci %d, r%d, %d\n", I5, ra, I11);
> + =A0 =A0 =A0 =A0{
> + =A0 =A0 =A0 =A0 =A0 =A0TCGv t1 =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0TCGv t2 =3D tcg_temp_new();
> + =A0 =A0 =A0 =A0 =A0 =A0TCGv ttmp =3D tcg_temp_new(); =A0 /* =A0store ma=
chi maclo*/
> + =A0 =A0 =A0 =A0 =A0 =A0ttmp =3D tcg_const_tl(tmp);

Leaked previous ttmp temporary.

> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_mul_tl(t0, cpu_R[ra], ttmp);

What t0?

> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ext_i32_i64(t1, t0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_concat_i32_i64(t2, maclo, machi);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_add_i64(t2, t2, t1);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_i32(maclo, t2);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_shri_i64(t2, t2, 32);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_i32(machi, t2);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t1);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t2);

Leaked ttmp temporary.

> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0break;

[...]

> + =A0 =A0case 0x20: =A0 /*l.ld*/
> + =A0 =A0 =A0 =A0LOG_DIS("l.ld r%d, r%d, %d\n", rd, ra, I16);
> + =A0 =A0 =A0 =A0tcg_gen_addi_tl(t0, cpu_R[ra], sign_extend(I16, 16));
> + =A0 =A0 =A0 =A0tcg_gen_qemu_ld64(cpu_R[rd], t0, dc->mem_idx);

Cannot ld64 into _tl register.

[...]

> + =A0 =A0case 0x34: =A0 /*l.sd*/
> + =A0 =A0 =A0 =A0LOG_DIS("l.sd %d, r%d, r%d, %d\n", I5, ra, rb, I11);
> + =A0 =A0 =A0 =A0tcg_gen_addi_tl(t0, cpu_R[ra], sign_extend(tmp, 16));
> + =A0 =A0 =A0 =A0tcg_gen_qemu_st64(cpu_R[rb], t0, dc->mem_idx);

Ditto.

[...]

> +static void dec_mac(DisasContext *dc, CPUOpenRISCState *env, uint32_t in=
sn)
> +{
> + =A0 =A0uint32_t op0;
> + =A0 =A0uint32_t ra, rb;
> + =A0 =A0op0 =3D field(insn, 0, 4);
> + =A0 =A0ra =3D field(insn, 16, 5);
> + =A0 =A0rb =3D field(insn, 11, 5);
> + =A0 =A0TCGv_i64 t0 =3D tcg_temp_new();
> + =A0 =A0TCGv_i64 t1 =3D tcg_temp_new();
> + =A0 =A0TCGv_i64 t2 =3D tcg_temp_new();
> + =A0 =A0switch (op0) {
> + =A0 =A0case 0x0001: =A0 /*l.mac*/
> + =A0 =A0 =A0 =A0LOG_DIS("l.mac r%d, r%d\n", ra, rb);
> + =A0 =A0 =A0 =A0{
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_mul_tl(t0, cpu_R[ra], cpu_R[rb]);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ext_i32_i64(t1, t0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_concat_i32_i64(t2, maclo, machi);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_add_i64(t2, t2, t1);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_i32(maclo, t2);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_shri_i64(t2, t2, 32);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_i32(machi, t2);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t1);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t2);

Instead of freeing temporaries repeatedly in some cases (and
leaking them in others) you could free them once after the switch.

> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0case 0x0002: =A0 /*l.msb*/
> + =A0 =A0 =A0 =A0LOG_DIS("l.msb r%d, r%d\n", ra, rb);
> + =A0 =A0 =A0 =A0{
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_mul_tl(t0, cpu_R[ra], cpu_R[rb]);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_ext_i32_i64(t1, t0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_concat_i32_i64(t2, maclo, machi);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_sub_i64(t2, t2, t1);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_i32(maclo, t2);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_shri_i64(t2, t2, 32);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_trunc_i64_i32(machi, t2);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t0);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t1);
> + =A0 =A0 =A0 =A0 =A0 =A0tcg_temp_free(t2);
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0break;
> +
> + =A0 =A0default:
> + =A0 =A0 =A0 =A0gen_illegal_exception(dc);
> + =A0 =A0 =A0 =A0break;
> + =A0 }
> +}

--=20
Thanks.
-- Max