Re: [Qemu-devel] [PATCH v5 2/7] host-utils: Implement unsigned quadword left/right shift and unit tests

[David Gibson]

On Tue, Jan 10, 2017 at 08:34:29AM -0600, Eric Blake wrote:
> On 01/09/2017 08:10 PM, Jose Ricardo Ziviani wrote:
> > Implements 128-bit left shift and right shift as well as their
> > testcases. By design, shift silently mods by 128, so the caller is
> > responsible to assert the shift range if necessary.
> > 
> > Left shift sets the overflow flag if any non-zero digit is shifted out.
> > 
> > Examples:
> >  ulshift(&low, &high, 250, &overflow);
> >  equivalent: n << 122
> > 
> >  urshift(&low, &high, -2);
> >  equivalent: n << 126
> > 
> > Signed-off-by: Jose Ricardo Ziviani <address@hidden>
> > ---
> 
> > +typedef struct {
> > +    uint64_t low;
> > +    uint64_t high;
> > +    uint64_t rlow;
> > +    uint64_t rhigh;
> > +    int32_t shift;
> > +    bool overflow;
> > +} test_data;
> > +
> > +static const test_data test_ltable[] = {
> > +    { 0x4C7ULL, 0x0ULL, 0x00000000000004C7ULL,
> > +      0x0000000000000000ULL,   0, false },
> 
> I might have laid it out as:
> 
> { 0x00000000000004c7ULL, 0x0000000000000000ULL,
>   0x00000000000004c7ULL, 0x0000000000000000ULL,
>   0, false }
> 
> to make the pre- and post-shift values line up better.  It's not fatal
> to the patch, so it's up to the maintainer if they want a v6 to improve
> the alignment.

host-utils doesn't have a maintainer.  So, I'm intending to take it
through my tree with your R-b.

> > +    { 0x8888888888888888ULL, 0x9999999999999999ULL,
> > +      0x8000000000000000ULL, 0x9888888888888888ULL, 60, true },
> > +    { 0x8888888888888888ULL, 0x9999999999999999ULL,
> > +      0x0000000000000000ULL, 0x8888888888888888ULL, 64, true },
> 
> These two are the most legible.
> 
> > +};
> > +
> > +static const test_data test_rtable[] = {
> 
> > +++ b/util/host-utils.c
> > @@ -161,3 +161,67 @@ int divs128(int64_t *plow, int64_t *phigh, int64_t 
> > divisor)
> >  }
> >  #endif
> >  
> > +/**
> > + * urshift - 128-bit Unsigned Right Shift.
> > + * @plow: in/out - lower 64-bit integer.
> > + * @phigh: in/out - higher 64-bit integer.
> > + * @shift: in - bytes to shift, between 0 and 127.
> > + *
> > + * Result is zero-extended and stored in plow/phigh, which are
> > + * input/output variables. Shift values outside the range will
> > + * be mod to 128. In other words, the caller is responsible to
> > + * verify/assert both the shift range and plow/phigh pointers.
> > + */
> 
> Duplicating docs in the .h and .c doesn't hurt, but risks one getting
> out of date; we have other spots that put the docs in the .h (where
> callers will look up what's available) or the .c (where the
> implementation is there to check against the docs). I don't have any
> strong preference on how to do it, though, so I don't mind leaving it as is.
> 
> Reviewed-by: Eric Blake <address@hidden>
> 




-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature