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

[Richard Henderson <rth@twiddle.net>]

On 10/24/2016 09:08 PM, Nikunj A Dadhania wrote:
> Richard Henderson <rth@twiddle.net> 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.

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


r~