From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=43048 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PfDUK-000674-5D
	for qemu-devel@nongnu.org; Tue, 18 Jan 2011 10:28:53 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1PfDTz-0004o1-UH
	for qemu-devel@nongnu.org; Tue, 18 Jan 2011 10:28:33 -0500
Received: from mail-wy0-f173.google.com ([74.125.82.173]:60903)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1PfDTz-0004nc-Q4
	for qemu-devel@nongnu.org; Tue, 18 Jan 2011 10:28:31 -0500
Received: by wyg36 with SMTP id 36so6705332wyg.4
	for <qemu-devel@nongnu.org>; Tue, 18 Jan 2011 07:28:30 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <4D35A4FB.3030403@st.com>
References: <4D35A4FB.3030403@st.com>
Date: Tue, 18 Jan 2011 15:26:44 +0000
Message-ID: <AANLkTikC-TeX9PmbU-GfLY-EKjC0kL4oET=J8XFhwzSh@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH] target-arm: Fix garbage collection of
	temporaries in Neon emulation.
From: Peter Maydell <peter.maydell@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
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: Christophe Lyon <christophe.lyon@st.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

On 18 January 2011 14:34, Christophe Lyon <christophe.lyon@st.com> wrote:
> +
> + =C2=A0 =C2=A0/* gen_helper_neon_mull_[su]{8|16} do not free their param=
eters.
> + =C2=A0 =C2=A0 =C2=A0 Don't forget to clean them now. =C2=A0*/
> + =C2=A0 =C2=A0switch ((size << 1) | u) {
> + =C2=A0 =C2=A0case 0:
> + =C2=A0 =C2=A0case 1:
> + =C2=A0 =C2=A0case 2:
> + =C2=A0 =C2=A0case 3:
> + =C2=A0 =C2=A0 =C2=A0dead_tmp(a);
> + =C2=A0 =C2=A0 =C2=A0dead_tmp(b);
> + =C2=A0 =C2=A0 =C2=A0break;
> + =C2=A0 =C2=A0}
> =C2=A0}

This seems a rather convoluted way to write "if (size < 2) { ... }"

> @@ -5235,9 +5245,12 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 tmp =3D neon_load_reg(rn, 0);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 } else {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 tmp =3D tmp3;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 /* tmp2 has been discarded in
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gen_neon_mull during pass 0, we need to
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0recreate it. =C2=A0*/
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 tmp2 =3D neon_get_scalar(size, rm);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 }

I think this will give the wrong results for instructions where the
scalar operand is in the same Neon register as the destination
for the first pass, because calling neon_get_scalar() again will
do a reload from the Neon register and it might have changed.
(Also loading it once at the start rather than in every pass is
more efficient as well as being correct :-))

Also your patch has hard-coded tabs in it (please see
CODING_STYLE on the subject of whitespace) and your
mail client or server has line-wrapped long lines in the patch
so it doesn't apply cleanly...

-- PMM