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

[David Gibson]

On Mon, Dec 05, 2016 at 07:35:39AM -0200, address@hidden wrote:
> On Mon, Dec 05, 2016 at 12:56:39PM +1100, David Gibson wrote:
> > On Sat, Dec 03, 2016 at 05:37:27PM -0800, Richard Henderson wrote:
> > > On 12/02/2016 09:00 PM, Jose Ricardo Ziviani wrote:
> > > > +++ b/include/qemu/host-utils.h
> > > > @@ -29,6 +29,33 @@
> > > >  #include "qemu/bswap.h"
> > > > 
> > > >  #ifdef CONFIG_INT128
> > > > +static inline void urshift(uint64_t *plow, uint64_t *phigh, uint32_t 
> > > > shift)
> > > > +{
> > > > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > > > +    val >>= (shift & 127);
> > > > +    *phigh = val >> 64;
> > > > +    *plow = val & 0xffffffffffffffff;
> > > > +}
> > > > +
> > > > +static inline void ulshift(uint64_t *plow, uint64_t *phigh,
> > > > +                           uint32_t shift, bool *overflow)
> > > > +{
> > > > +    __uint128_t val = ((__uint128_t)*phigh << 64) | *plow;
> > > > +
> > > > +    if (shift == 0) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (shift > 127 || (val >> (128 - (shift & 127))) != 0) {
> > > > +        *overflow = true;
> > > > +    }
> > > > +
> > > > +    val <<= (shift & 127);
> > > > +
> > > > +    *phigh = val >> 64;
> > > > +    *plow = val & 0xffffffffffffffff;
> > > > +}
> > > > +
> > > 
> > > This belongs in qemu/int128.h, not here.  And certainly not predicated on
> > > CONFIG_INT128.
> > 
> > Is there actually any advantage to the __uint128_t based versions over
> > the 64-bit versions?
> 
> Nothing special here. It just looks more clear (to me) to shift all 128
> bits at once than 2x64. But I agree we won't loose to have just one
> function outside CONFIG_INT128.

It is clearer, but having two different version makes things less
clear again.  We need to have the 64-bit only version for compilers
without int128_t support, so..

> So, I'll remove these two functions and keep only the other two using
> uint64_t types.
> 
> Anyway I get a bit confused about int128.h and host-utils.h. I see
> functions like divu128 and divs128 that could be in int128.h, since
> there is no similar operation in int128.h. Is there any rule about it?
> 
> Thank you guys for reviewing it!
> 
> > 
> 
> 

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