From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38899)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <James.Hogan@imgtec.com>) id 1XgxrH-0003vd-6R
	for qemu-devel@nongnu.org; Wed, 22 Oct 2014 11:30:01 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <James.Hogan@imgtec.com>) id 1XgxrB-0002fI-1Y
	for qemu-devel@nongnu.org; Wed, 22 Oct 2014 11:29:55 -0400
Received: from mailapp01.imgtec.com ([195.59.15.196]:58889)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <James.Hogan@imgtec.com>) id 1XgxrA-0002f8-SQ
	for qemu-devel@nongnu.org; Wed, 22 Oct 2014 11:29:48 -0400
Message-ID: <5447CD69.5090703@imgtec.com>
Date: Wed, 22 Oct 2014 16:29:45 +0100
From: James Hogan <james.hogan@imgtec.com>
MIME-Version: 1.0
References: <1405331763-57126-1-git-send-email-yongbok.kim@imgtec.com>
	<1405331763-57126-9-git-send-email-yongbok.kim@imgtec.com>
In-Reply-To: <1405331763-57126-9-git-send-email-yongbok.kim@imgtec.com>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 08/20] target-mips: add msa_helper.c
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
Cc: cristian.cuna@imgtec.com, leon.alrae@imgtec.com, aurelien@aurel32.net

On 14/07/14 10:55, Yongbok Kim wrote:
> +#define  B(pwr, i) (((wr_t *)pwr)->b[i])
> +#define BR(pwr, i) (((wr_t *)pwr)->b[i])
> +#define BL(pwr, i) (((wr_t *)pwr)->b[i + MSA_WRLEN/16])

macro argument references should be enclosed in brackets really (to
avoid precedence problems).

> +
> +#define ALL_B_ELEMENTS(i, wrlen)                \
> +    do {                                        \
> +        uint32_t i;                             \
> +        for (i = wrlen / 8; i--;)

eww... there's gotta be a nicer way.

Is it really so long winded not to do directly?
	int i;
	for (i = 0; i < MSA_WRLEN/8; ++i) {

	}

compared to what you have at the moment:
	ALL_B_ELEMENTS(i, MSA_WRLEN) {

	} DONE_ALL_ELEMENTS;

It would be much more familiar/readable, and the ordering is explicit
too (just in case it matters for any vector operations when source ==
destination)

> +static inline void msa_move_v(void *pwd, void *pws)

why not s/void/wr_t/?

You could then presumably do *pwd = *pws

> +{
> +    ALL_D_ELEMENTS(i, MSA_WRLEN) {
> +        D(pwd, i) = D(pws, i);
> +    } DONE_ALL_ELEMENTS;
> +}

Cheers
James