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

[Richard Henderson]

On 1/29/19 9:10 PM, Mark Cave-Ayland wrote:
>> 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 nevermind.
> 
> 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;                                                      \
>         }                                                                    \

Why are you over-complicating things?  I thought I'd explicitly said this
twice, but perhaps not:

Pass the symbol "half" directly to VMRG_DO:

#define VMRG(suffix, element, access)          \
    VMRG_DO(mrgl##suffix, element, access, 0)  \
    VMRG_DO(mrgh##suffix, element, access, half)

You do not need another conditional within VMRG_DO.

Also, I'm pretty sure it's the "low" merge that wants the "ofs" to be non-zero,
since all of the Vsr* symbols implement big-endian indexing.


r~