Richard Henderson <r...@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.
>> +#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
