Re: [PATCH] x86: merge the simple bitops and move them to bitops.h

["Alexander van Heukelum" <heukelum@fastmail.fm>]

On Fri, 14 Mar 2008 11:07:55 -0700, "Jeremy Fitzhardinge"
<jeremy@goop.org> said:
> Alexander van Heukelum wrote:
> > x86: merge the simple bitops and move them to bitops.h
> >
> > Some of those can be written in such a way that the same
> > inline assembly can be used to generate both 32 bit and
> > 64 bit code.
> >
> > For ffs and fls, x86_64 unconditionally used the cmov
> > instruction and i386 unconditionally used a conditional
> > branch over a mov instruction. In the current patch I
> > chose to select the version based on the availability
> > of the cmov instruction instead. A small detail here is
> > that x86_64 did not previously set CONFIG_X86_CMOV=y.
> >   
> 
> Looks good in general.  What's left in bitops_{32,64}.h now?

Thanks for taking a look!

bitops_{32,64}.h are getting pretty empty ;)

Both contain find_first_bit/find_first_zero_bit, i386 has them inlined,
x86_64 has an ugly define to select between small bitmaps (inlined) and
an out-of-line version. I think they should be unified much like how
find_next_bit and find_next_zero_bit work now (in x86#testing).

Both define fls64(), but i386 uses a generic one and x86_64 defines
one all by itself. The generic one is currently not suitable for
use by 64-bit archs... that can change.

x86_64 defines ARCH_HAS_FAST_MULTIPLIER, i386 not. This affects a
choice of generated code in the (generic) hweight function. It would
be nice if that could move to some other file.

x86_64 has a mysterious inline function set_bit_string, which is
only used by pci-calgary_64.c and pci-gart_64.c. Not sure what to
do with it.

> (Some comments below.)

--- %< ---

> > +#ifdef __KERNEL__
> > +/**
> > + * ffs - find first bit set
> > + * @x: the word to search
> > + *
> > + * This is defined the same way as
> > + * the libc and compiler builtin ffs routines, therefore
> > + * differs in spirit from the above ffz() (man ffs).
> 
> This comment seems wrong.  My "man ffs" says that it returns 1-32 for 
> non-zero inputs, and 0 for a zero input.  This function returns 0-31, or 
> -1 for a zero input.

Seems, indeed. You missed the "return r + 1;" ;-)

> > + */
> > +static inline int ffs(int x)
> > +{
> > +	int r;
> > +#ifdef CONFIG_X86_CMOV
> > +	__asm__("bsfl %1,%0\n\t"
> > +		"cmovzl %2,%0"
> >   
> 
> The prevailing style in bitops.h is to use "bsf"/"cmovz" and let the 
> compiler work out the size from the register names.

The comment rightly mentions that ffs differs in spirit from the rest
of the functions here. The type to be used here it always a 4-byte one
(int); most other functions operate on longs. I have not checked if the
explicit size is absolutely necessary, but I feel much safer with the
explicit size there.

> > +		: "=r" (r) : "rm" (x), "r" (-1));
> > +#else
> > +	__asm__("bsfl %1,%0\n\t"
> > +		"jnz 1f\n\t"
> > +		"movl $-1,%0\n"
> > +		"1:" : "=r" (r) : "rm" (x));
> > +#endif
> > +	return r+1;
> > +}
> > +
> > +/**
> > + * fls - find last bit set
> > + * @x: the word to search
> > + *
> > + * This is defined the same way as ffs().
> >   
> 
> And this comment is even more wrong, given that ffs and fls are 
> completely different functions ;)
> 
> I know these are from the original, but its worth fixing given that 
> you're touching it anyway.

I'll see if I can come up with something better.

Greetings,
    Alexander

> > + */
> > +static inline int fls(int x)
> > +{
> > +	int r;
> > +#ifdef CONFIG_X86_CMOV
> > +	__asm__("bsrl %1,%0\n\t"
> > +		"cmovzl %2,%0"
> > +		: "=&r" (r) : "rm" (x), "rm" (-1));
> > +#else
> > +	__asm__("bsrl %1,%0\n\t"
> > +		"jnz 1f\n\t"
> > +		"movl $-1,%0\n"
> > +		"1:" : "=r" (r) : "rm" (x));
> > +#endif
> > +	return r+1;
> > +}
> > +#endif /* __KERNEL__ */
> > +
> >  #undef ADDR
> >  
> >  #ifdef CONFIG_X86_32
> 
>     J
-- 
  Alexander van Heukelum
  heukelum@fastmail.fm

-- 
http://www.fastmail.fm - A no graphics, no pop-ups email service