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 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.

>> +#define MASK(size, max_val)                                     \
>> +static inline uint##size##_t mask_u##size(uint##size##_t start, \
>> +                                uint##size##_t end)             \
>> +{                                                               \
>> +    uint##size##_t ret, max_bit = size - 1;                     \
>> +                                                                \
>> +    if (likely(start == 0)) {                                   \
>> +        ret = max_val << (max_bit - end);                       \
>> +    } else if (likely(end == max_bit)) {                        \
>> +        ret = max_val >> start;                                 \
>> +    } else {                                                    \
>> +        ret = (((uint##size##_t)(-1ULL)) >> (start)) ^          \
>> +            (((uint##size##_t)(-1ULL) >> (end)) >> 1);          \
>> +        if (unlikely(start > end)) {                            \
>> +            return ~ret;                                        \
>> +        }                                                       \
>> +    }                                                           \
>
> Why the two likely cases?  Doesn't the third case cover them?
>
> Also, (uint##size##_t)(-1ULL) should be just (uint##size##_t)-1.
> Please remove all the other unnecessarry parenthesis too.
>
> Hmph.  I see you've copied all this silliness from translate.c, so...
> nevermind, I guess.  Let's leave this a near-exact copy.

Ok.

>> +#define LEFT_ROTATE(size)                                            \
>> +static inline uint##size##_t left_rotate_u##size(uint##size##_t val, \
>> +                                              uint##size##_t shift)  \
>> +{                                                                    \
>> +    if (!shift) {                                                    \
>> +        return val;                                                  \
>> +    }                                                                \
>> +                                                                     \
>> +    uint##size##_t left_val = extract_bits_u##size(val, 0, shift - 1); \
>> +    uint##size##_t right_val = val & mask_u##size(shift, size - 1);    \
>> +                                                                     \
>> +    return right_val << shift | left_val;                            \
>> +}
>> +
>> +LEFT_ROTATE(32);
>> +LEFT_ROTATE(64);
>
> We already have rol32 and rol64.
>
> Which I see are broken for shift == 0.  Let's please fix that, as a separate
> patch, like so:
>
>   return (word << shift) | (word >> ((32 - shift) & 31));

Sure.

Regards
Nikunj