bug in arch/ppc/kernel/misc.S: __ashrdi3?

bug in arch/ppc/kernel/misc.S: __ashrdi3?

[Frank van Maarseveen]

I don't really grok the code but an operand seems to be missing and the
assembler makes something out of it I don't trust:

_GLOBAL(__ashrdi3)
	...
	rlwinm  r8,r7,0,32      # t3 = (count < 32) ? 32 : 0

Trying it out:

$ cat a.c
void f(void)
{
        __asm("rlwinm  8,7,0,32");
}
$ ppc_4xx-gcc -O2 -c a.c
$ ppc-linux-objdump -S a.o

a.o:     file format elf32-powerpc

Disassembly of section .text:

00000000 <f>:
f():
   0:   54 e8 06 b4     rlwinm  r8,r7,0,26,26
   4:   4e 80 00 20     blr

-- 
Frank

Re: bug in arch/ppc/kernel/misc.S: __ashrdi3?

[Andreas Schwab]

Frank van Maarseveen <frankvm@frankvm.com> writes:

> I don't really grok the code but an operand seems to be missing and the
> assembler makes something out of it I don't trust:
>
> _GLOBAL(__ashrdi3)
> 	...
> 	rlwinm  r8,r7,0,32      # t3 = (count < 32) ? 32 : 0

32 == 0x80000000 >> 26

> 00000000 <f>:
> f():
>    0:   54 e8 06 b4     rlwinm  r8,r7,0,26,26

The mask begins at bit 26 and ends at bit 26 (counted from the left).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: bug in arch/ppc/kernel/misc.S: __ashrdi3?

[Benjamin Herrenschmidt]

On Fri, 2005-07-15 at 18:01 +0200, Frank van Maarseveen wrote:
> I don't really grok the code but an operand seems to be missing and the
> assembler makes something out of it I don't trust:
> 
> _GLOBAL(__ashrdi3)
> 	...
> 	rlwinm  r8,r7,0,32      # t3 = (count < 32) ? 32 : 0

This is equivalent to r8 = r7 & 32. It will definitely not do what the
comment says though. If (count < 64), however, it will do something
like r8 = (r7 < 32) ? 0 : 32. Paul, maybe we should dbl check what's
going in there ?

>    0:   54 e8 06 b4     rlwinm  r8,r7,0,26,26

This is the same.

The assembler, when provided with only one argument after the shift
count, will generate ME and MB to represent the mask whose value is
given in argument. (If you give an argument which doesn't contain a
single contiguous range of 1 bits, I suppose it will error out).

Ben.

Re: bug in arch/ppc/kernel/misc.S: __ashrdi3?

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Fri, 2005-07-15 at 18:01 +0200, Frank van Maarseveen wrote:
>> I don't really grok the code but an operand seems to be missing and the
>> assembler makes something out of it I don't trust:
>> 
>> _GLOBAL(__ashrdi3)
>> 	...
>> 	rlwinm  r8,r7,0,32      # t3 = (count < 32) ? 32 : 0
>
> This is equivalent to r8 = r7 & 32. It will definitely not do what the
> comment says though. If (count < 64), however, it will do something
> like r8 = (r7 < 32) ? 0 : 32. Paul, maybe we should dbl check what's
> going in there ?

r7 is count + 32, and ((count + 32) & 32) is equivalent to the expression
above if count < 64.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: bug in arch/ppc/kernel/misc.S: __ashrdi3?

[Benjamin Herrenschmidt]

On Sat, 2005-07-16 at 20:02 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > On Fri, 2005-07-15 at 18:01 +0200, Frank van Maarseveen wrote:
> >> I don't really grok the code but an operand seems to be missing and the
> >> assembler makes something out of it I don't trust:
> >> 
> >> _GLOBAL(__ashrdi3)
> >> 	...
> >> 	rlwinm  r8,r7,0,32      # t3 = (count < 32) ? 32 : 0
> >
> > This is equivalent to r8 = r7 & 32. It will definitely not do what the
> > comment says though. If (count < 64), however, it will do something
> > like r8 = (r7 < 32) ? 0 : 32. Paul, maybe we should dbl check what's
> > going in there ?
> 
> r7 is count + 32, and ((count + 32) & 32) is equivalent to the expression
> above if count < 64.

Ok, with some context it makes more sense :)

Ben.

Re: bug in arch/ppc/kernel/misc.S: __ashrdi3?

[Gabriel Paubert]

On Sun, Jul 17, 2005 at 04:24:57AM +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2005-07-16 at 20:02 +0200, Andreas Schwab wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> > 
> > > On Fri, 2005-07-15 at 18:01 +0200, Frank van Maarseveen wrote:
> > >> I don't really grok the code but an operand seems to be missing and the
> > >> assembler makes something out of it I don't trust:
> > >> 
> > >> _GLOBAL(__ashrdi3)
> > >> 	...
> > >> 	rlwinm  r8,r7,0,32      # t3 = (count < 32) ? 32 : 0
> > >
> > > This is equivalent to r8 = r7 & 32. It will definitely not do what the
> > > comment says though. If (count < 64), however, it will do something
> > > like r8 = (r7 < 32) ? 0 : 32. Paul, maybe we should dbl check what's
> > > going in there ?
> > 
> > r7 is count + 32, and ((count + 32) & 32) is equivalent to the expression
> > above if count < 64.
> 
> Ok, with some context it makes more sense :)

Well, I wrote this code, to be fair I actually took most of these 
code sequences from the multi precision shift appendix found in
all good PPC instruction set books. The exception is __ashrdi3 
which I modified slightly to eliminate a conditional branch.

I tested it fairly exhaustively before submitting it. So the 
comments might be wrong (and they will definitely look wrong
without the correct context), but the code very likely gives 
the correct result. After all it has been used for quite some 
time now without any complaint. 

Now obviously the function only works for shift counts below 
64, since the C language specification clearly states that 
the result is undefined for shift counts equal or larger than 
the bit size of the operand (but I felt against rebooting the
machine for an operand outside the allowed range, although
a simple "twlgti count,63" might do it).

Maybe this last point should be clearly stated in a comment.

	Regards,
	Gabriel