[2.6.6-BK] x86_64 has buggy ffs() implementation

[2.6.6-BK] x86_64 has buggy ffs() implementation

[Anton Altaparmakov]

Hi Andi, Andrew, Linus,

x86_64 has incorrect include/asm-x86_64/bitops.h::ffs() implementation. 
It uses "g" instead of "rm" in the insline assembled bsfl instruction. 
(This was spotted by Yuri Per.)

bsfl does not accept constant values but only memory ones.  On i386 the
correct "rm" is used.

This causes NTFS build to fail as gcc optimizes a variable into a
constant and ffs() then fails to assemble.

Please apply below patch.  Thanks!

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ &
http://www-stu.christs.cam.ac.uk/~aia21/

--- bklinux-2.6/include/asm-x86_64/bitops.h.old	2004-05-12
14:40:32.524022336 +0100
+++ bklinux-2.6/include/asm-x86_64/bitops.h	2004-05-12
14:41:22.595410328 +0100
@@ -458,7 +458,7 @@ static __inline__ int ffs(int x)
 
 	__asm__("bsfl %1,%0\n\t"
 		"cmovzl %2,%0" 
-		: "=r" (r) : "g" (x), "r" (-1));
+		: "=r" (r) : "rm" (x), "r" (-1));
 	return r+1;
 }
 

Re: [2.6.6-BK] x86_64 has buggy ffs() implementation

[Andi Kleen]

On Wed, 12 May 2004 14:43:37 +0100
Anton Altaparmakov <aia21@cam.ac.uk> wrote:

> Hi Andi, Andrew, Linus,
> 
> x86_64 has incorrect include/asm-x86_64/bitops.h::ffs() implementation. 
> It uses "g" instead of "rm" in the insline assembled bsfl instruction. 
> (This was spotted by Yuri Per.)
> 
> bsfl does not accept constant values but only memory ones.  On i386 the
> correct "rm" is used.
> 
> This causes NTFS build to fail as gcc optimizes a variable into a
> constant and ffs() then fails to assemble.
> 
> Please apply below patch.  Thanks!

Thanks. I applied it to my tree.

-Andi

Re: [2.6.6-BK] x86_64 has buggy ffs() implementation

[H. Peter Anvin]

Followup to:  <1084369416.16624.53.camel@imp.csi.cam.ac.uk>
By author:    Anton Altaparmakov <aia21@cam.ac.uk>
In newsgroup: linux.dev.kernel
>
> Hi Andi, Andrew, Linus,
> 
> x86_64 has incorrect include/asm-x86_64/bitops.h::ffs() implementation. 
> It uses "g" instead of "rm" in the insline assembled bsfl instruction. 
> (This was spotted by Yuri Per.)
> 
> bsfl does not accept constant values but only memory ones.  On i386 the
> correct "rm" is used.
> 
> This causes NTFS build to fail as gcc optimizes a variable into a
> constant and ffs() then fails to assemble.
> 

Of course, this is a good reason to do a __builtin_constant_p()
wrapper that gcc can optimize:

static __inline__ __attribute_const__ int ffs(int x)
{  
	if ( __builtin_constant_p(x) ) {
		unsigned int y = (unsigned int)x;
		if ( y >= 0x80000000 )
			return 32;
		else if ( y >= 0x40000000 )
			return 31;
		else if /* ... you get the idea ... */
	} else {
		__asm__("bsfl %1,%0\n\t"
			"cmovzl %2,%0" 
			: "=r" (r) : "rm" (x), "r" (-1));
		return r+1;
	}
}

	-hpa

Re: [2.6.6-BK] x86_64 has buggy ffs() implementation

[Anton Altaparmakov]

On Wed, 12 May 2004, H. Peter Anvin wrote:
> Followup to:  <1084369416.16624.53.camel@imp.csi.cam.ac.uk>
> By author:    Anton Altaparmakov <aia21@cam.ac.uk>
> In newsgroup: linux.dev.kernel
> >
> > Hi Andi, Andrew, Linus,
> >
> > x86_64 has incorrect include/asm-x86_64/bitops.h::ffs() implementation.
> > It uses "g" instead of "rm" in the insline assembled bsfl instruction.
> > (This was spotted by Yuri Per.)
> >
> > bsfl does not accept constant values but only memory ones.  On i386 the
> > correct "rm" is used.
> >
> > This causes NTFS build to fail as gcc optimizes a variable into a
> > constant and ffs() then fails to assemble.
> >
>
> Of course, this is a good reason to do a __builtin_constant_p()
> wrapper that gcc can optimize:
>
> static __inline__ __attribute_const__ int ffs(int x)
> {
> 	if ( __builtin_constant_p(x) ) {
> 		unsigned int y = (unsigned int)x;
> 		if ( y >= 0x80000000 )
> 			return 32;
> 		else if ( y >= 0x40000000 )
> 			return 31;
> 		else if /* ... you get the idea ... */

If you are going to play with that why not just use generic_ffs() instead
of doing it by hand?

Best regards,

	Anton

> 	} else {
> 		__asm__("bsfl %1,%0\n\t"
> 			"cmovzl %2,%0"
> 			: "=r" (r) : "rm" (x), "r" (-1));
> 		return r+1;
> 	}
> }

-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [2.6.6-BK] x86_64 has buggy ffs() implementation

[Gabriel Paubert]

On Wed, May 12, 2004 at 08:31:56PM +0000, H. Peter Anvin wrote:
> Followup to:  <1084369416.16624.53.camel@imp.csi.cam.ac.uk>
> By author:    Anton Altaparmakov <aia21@cam.ac.uk>
> In newsgroup: linux.dev.kernel
> >
> > Hi Andi, Andrew, Linus,
> > 
> > x86_64 has incorrect include/asm-x86_64/bitops.h::ffs() implementation. 
> > It uses "g" instead of "rm" in the insline assembled bsfl instruction. 
> > (This was spotted by Yuri Per.)
> > 
> > bsfl does not accept constant values but only memory ones.  On i386 the
> > correct "rm" is used.
> > 
> > This causes NTFS build to fail as gcc optimizes a variable into a
> > constant and ffs() then fails to assemble.
> > 
> 
> Of course, this is a good reason to do a __builtin_constant_p()
> wrapper that gcc can optimize:
> 
> static __inline__ __attribute_const__ int ffs(int x)
> {  
> 	if ( __builtin_constant_p(x) ) {
> 		unsigned int y = (unsigned int)x;
> 		if ( y >= 0x80000000 )
> 			return 32;
> 		else if ( y >= 0x40000000 )
> 			return 31;
> 		else if /* ... you get the idea ... */

Either I'm asleep or you are emulating bsrl, not bsfl. It
should rather be:

	if ( y & 0x00000001) return 1;
	if ( y & 0x00000002) return 2;
	if ( y & 0x00000004) return 3;
	...
	if ( y & 0x80000000) return 32;
	return 0;

No need for the else clauses either because of the return.
But maybe even __builtin_ffs(y) would work in this case.

	Gabriel

Re: [2.6.6-BK] x86_64 has buggy ffs() implementation

[H. Peter Anvin]

Gabriel Paubert wrote:
> 
> Either I'm asleep or you are emulating bsrl, not bsfl. It
> should rather be:
> 
> 	if ( y & 0x00000001) return 1;
> 	if ( y & 0x00000002) return 2;
> 	if ( y & 0x00000004) return 3;
> 	...
> 	if ( y & 0x80000000) return 32;
> 	return 0;
> 
> No need for the else clauses either because of the return.
> But maybe even __builtin_ffs(y) would work in this case.
> 

If __builtin_ffs() works *AND HAS THE RIGHT SEMANTICS* it's probably the 
best thing to use.

Otherwise, yes, generic_ffs() can clearly be used inside the 
__builtin_constant_p() clause.

	-hpa