Re: [Qemu-devel] [PATCH 08/20] target-mips: add msa_helper.c

[James Hogan]

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