Re: [Qemu-ppc] [PATCH 3/4] target-ppc: add vrldnmi and vrlwmi instructions

[Nikunj A Dadhania]

Richard Henderson <address@hidden> writes:

> On 10/24/2016 09:08 PM, Nikunj A Dadhania wrote:
>> Richard Henderson <address@hidden> writes:
>>
>>> On 10/24/2016 02:14 AM, Nikunj A Dadhania wrote:
>>>> +#define EXTRACT_BITS(size)                                              \
>>>> +static inline uint##size##_t extract_bits_u##size(uint##size##_t reg,   \
>>>> +                                                  uint##size##_t start, \
>>>> +                                                  uint##size##_t end)   \
>>>> +{                                                                       \
>>>> +    uint##size##_t nr_mask_bits = end - start + 1;                      \
>>>> +    uint##size##_t val = 1;                                             \
>>>> +    uint##size##_t mask = (val << nr_mask_bits) - 1;                    \
>>>> +    uint##size##_t shifted_reg = reg  >> ((size - 1)  - end);           \
>>>> +    return shifted_reg & mask;                                          \
>>>> +}
>>>> +
>>>> +EXTRACT_BITS(64);
>>>> +EXTRACT_BITS(32);
>>>
>>> We already have extract32 and extract64, which you're (nearly) duplicating.
>>
>> The bit position number notation is different, because of this using the
>> above routine, MSB=0 and LSB=63.
>>
>> While the below assumes: MSB=63 and LSB=0
>>
>> static inline uint64_t extract64(uint64_t value, int start, int length)
>> {
>>     assert(start >= 0 && length > 0 && length <= 64 - start);
>>     return (value >> start) & (~0ULL >> (64 - length));
>> }
>>
>> Let me know if I am missing something here.
>
> Since the arguments to extract_bits_uN are completely under your control, via 
> the arguments to VRLMI, this is a non-argument.  Just change them to 
> little-endian position + length.

Sure, was already trying that, I have the changed version now:

#define VRLMI(name, size, element,                                  \
              begin_last,                                           \
              end_last,                                             \
              shift_last, num_bits, insert)                         \
void helper_##name(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)        \
{                                                                   \
    int i;                                                          \
    for (i = 0; i < ARRAY_SIZE(r->element); i++) {                  \
        uint##size##_t src1 = a->element[i];                        \
        uint##size##_t src2 = b->element[i];                        \
        uint##size##_t src3 = r->element[i];                        \
        uint##size##_t begin, end, shift, mask, rot_val;            \
                                                                    \
        begin = extract##size(src2, size - begin_last - 1, num_bits);   \
        end = extract##size(src2, size - end_last - 1, num_bits);       \
        shift = extract##size(src2, size - shift_last - 1, num_bits);   \
        rot_val = rol##size(src1, shift);                               \
        mask = mask_u##size(begin, end);                            \
        if (insert) {                                               \
            r->element[i] = (rot_val & mask) | (src3 & ~mask);      \
        } else {                                                    \
            r->element[i] = (rot_val & mask);                       \
        }                                                           \
    }                                                               \
}

VRLMI(vrldmi, 64, u64,
      47,  /* begin_last */
      55,  /* end_last */
      63,  /* shift_last */
      6,   /* num_bits */
      1);  /* mask and insert */

>
> (And, after you do that conversion for vrldmi and vilwmi, you'll see why 
> big-endian bit numbering is the spawn of the devil.  ;-)

That bit numbering gives nightmares ;-)

Regards
Nikunj