Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 2/8] target/ppc: rework vmrg{l, h}{b, h, w} instructions to use Vsr* macros

[BALATON Zoltan]

On Wed, 30 Jan 2019, Mark Cave-Ayland wrote:
> On 29/01/2019 23:05, Richard Henderson wrote:
>
> > On 1/29/19 11:17 AM, Mark Cave-Ayland wrote:
> > > +#define VMRG_DO(name, element, access, ofs)                             \
> > >      void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)       \
> > >      {                                                                   \
> > >          ppc_avr_t result;                                               \
> > >          int i;                                                          \
> > >                                                                          \
> > > +        for (i = 0; i < ARRAY_SIZE(r->element) / 2; i++) {              \
> > > +            result.access(i * 2 + 0) = a->access(i + ofs);              \
> > > +            result.access(i * 2 + 1) = b->access(i + ofs);              \
> > >          }                                                               \
> > >          *r = result;                                                    \
> > >      }
> > > +
> > > +#define VMRG(suffix, element, access, half)        \
> > > +    VMRG_DO(mrgl##suffix, element, access, half)   \
> > > +    VMRG_DO(mrgh##suffix, element, access, 0)
> > > +VMRG(b, u8, VsrB, 8)
> > > +VMRG(h, u16, VsrH, 4)
> > > +VMRG(w, u32, VsrW, 2)
> >
> > So... the point of "half" was to not replicate knowledge out to VMRG.
> > Just use
> >
> >     int i, half = ARRAY_SIZE(r->element) / 2;
> >     for (i = 0; i < half; i++) {
> >
> > within VMRG_DO.
>
> Okay - I was a bit confused because in your example macro signature you added 
> ofs
> which made me think you wanted its value to be determined outside, but never 
> mind.
>
> What about the following instead? With high set as part of the macro then the 
> initial
> assignment to ofs should be inlined accordingly at compile time:
>
>
> #define VMRG_DO(name, element, access, high)                                 \
>    void helper_v##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)            \
>    {                                                                        \
>        ppc_avr_t result;                                                    \
>        int ofs, half = ARRAY_SIZE(r->element) / 2;                          \
>        int i;                                                               \
>                                                                             \
>        if (high) {                                                          \
>            ofs = 0;                                                         \
>        } else {                                                             \
>            ofs = half;                                                      \
>        }                                                                    \

This could be also written as:

int i, half = ARRAY_SIZE(r->element) / 2;                            \
int ofs = (high ? 0 : half);                                         \

to save a few lines but it depends on personal taste which one is more 
readable for someone.

Regards,
BALATON Zoltan

>                                                                             \
>        for (i = 0; i < half; i++) {                                         \
>            result.access(i * 2 + 0) = a->access(i + ofs);                   \
>            result.access(i * 2 + 1) = b->access(i + ofs);                   \
>        }                                                                    \
>        *r = result;                                                         \
>    }
>
> #define VMRG(suffix, element, access)          \
>    VMRG_DO(mrgl##suffix, element, access, 0)  \
>    VMRG_DO(mrgh##suffix, element, access, 1)
> VMRG(b, u8, VsrB)
> VMRG(h, u16, VsrH)
> VMRG(w, u32, VsrW)
> #undef VMRG_DO
> #undef VMRG
> #undef VMRG
>
>
> ATB,
>
> Mark.