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

[Richard Henderson]

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.

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

> +#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));


r~