raid 5 with >= 5 members broken on x86

raid 5 with >= 5 members broken on x86

[Alexandre Oliva]

[Apologies if this is a dup (or a trip :-); I seem to be having
 trouble getting through the list spam filtering.  Too bad the failure
 is silent.]

There was a bug in the inline asm used to implement xor_p5_mmx_5.  It
had pushes and pops although it took a +g operand.  It turned out
that gcc chose to pass `lines' as an %esp+offset address.  With
changes in esp, it wouldn't work.

I suppose I could just change lines from +g to +r, like xor_pII_mmx_5,
but avoiding the pushes and pops is more efficient, and making sure
GCC doesn't get clever about sharing or reusing p4 and p5, it's just
as safe.  This approach should probably be extended to the other uses
of push and pop due to limitations in the number of operands.

Yet another possibility is to just use +r for p4 and p5; this works in
GCC 3.1 and above.  I wasn't sure the kernel was willing to require
that, so I took the most conservative approach.

Here's the patch.  More details at
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=116679

Here's the patch.

--- include/asm-i386/xor.h.orig	2004-02-26 15:13:03.000000000 -0300
+++ include/asm-i386/xor.h	2004-02-26 15:18:20.000000000 -0300
@@ -425,10 +425,28 @@
 
 	kernel_fpu_begin();
 
-	/* need to save p4/p5 manually to not exceed gcc's 10 argument limit */
+	/* GCC up to 3.0.x had this limitation of at most 10 asm
+	   operands, and + operands counted as two (one input and one
+	   output).  We unfortunately have to inform GCC that we're
+	   modifying all of these registers.  The trick we use here is
+	   to make sure the values of p4 and p5 are dissociated from
+	   whatever other registers or stack locations they might have
+	   been shared with, and make sure GCC knows they've been
+	   modified after the asm statement.  This incurs no
+	   additional costs and AFAICT is safe.
+
+	   Should it be found to not work, we'd have to resort to
+	   passing to the asm statement a pointer to a stack save
+	   area, and use that to preserve registers that had to be
+	   marked as read only but that may have been clobbered, which
+	   would impact performance without any actual advantage.
+	   Note that we can't just push and pop the registers, because
+	   this changes esp, and lines may be passed as an
+	   esp-relative address.  Well, I suppose we could require
+	   lines to be passed in a register...  -aoliva@redhat.com */
+
+	__asm__ ("" : "+r" (p4), "+r" (p5));
 	__asm__ __volatile__ (
-	"	pushl %4\n"
-	"	pushl %5\n"        	
 	" .align 32,0x90             ;\n"
 	" 1:                         ;\n"
 	"       movq   (%1), %%mm0   ;\n"
@@ -487,12 +505,11 @@
 	"       addl $64, %5         ;\n"
 	"       decl %0              ;\n"
 	"       jnz 1b               ;\n"
-	"	popl %5\n"
-	"	popl %4\n"
 	: "+g" (lines),
 	  "+r" (p1), "+r" (p2), "+r" (p3)
 	: "r" (p4), "r" (p5)
 	: "memory");
+	__asm__ __volatile__ ("" : "+r" (p4), "+r" (p5));
 
 	kernel_fpu_end();
 }

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Happy GNU Year!                     oliva@{lsd.ic.unicamp.br, gnu.org}
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: raid 5 with >= 5 members broken on x86

[Linus Torvalds]

On Thu, 26 Feb 2004, Alexandre Oliva wrote:
> 
> I suppose I could just change lines from +g to +r, like xor_pII_mmx_5,
> but avoiding the pushes and pops is more efficient, and making sure
> GCC doesn't get clever about sharing or reusing p4 and p5, it's just
> as safe.  This approach should probably be extended to the other uses
> of push and pop due to limitations in the number of operands.

You can't do this in a separate inline asm. There is nothing to say that 
gcc wouldn't do a re-load or something in between, so you really need to 
tell the _first_ ask about it.

> Yet another possibility is to just use +r for p4 and p5; this works in
> GCC 3.1 and above.  I wasn't sure the kernel was willing to require
> that, so I took the most conservative approach.

No, I don't think we're ready to force a bigger and slower compiler on x86 
for something like this. But your fix doesn't really work either.

One approach is to just do the loop _outside_ of the asm? I don't see much 
point to trying to force the small stuff. What's the difference if you do 
something like the appended?

Btw, the "xor_pII_mmx_5()" thing just uses "+r" for the line count, so why 
doesn't that work for this case?

		Linus

===== include/asm-i386/xor.h 1.14 vs edited =====
--- 1.14/include/asm-i386/xor.h	Tue Mar 11 18:15:03 2003
+++ edited/include/asm-i386/xor.h	Thu Feb 26 13:46:49 2004
@@ -426,74 +426,69 @@
 	kernel_fpu_begin();
 
 	/* need to save p4/p5 manually to not exceed gcc's 10 argument limit */
-	__asm__ __volatile__ (
-	"	pushl %4\n"
-	"	pushl %5\n"        	
-	" .align 32,0x90             ;\n"
-	" 1:                         ;\n"
-	"       movq   (%1), %%mm0   ;\n"
-	"       movq  8(%1), %%mm1   ;\n"
-	"       pxor   (%2), %%mm0   ;\n"
-	"       pxor  8(%2), %%mm1   ;\n"
-	"       movq 16(%1), %%mm2   ;\n"
-	"       pxor   (%3), %%mm0   ;\n"
-	"       pxor  8(%3), %%mm1   ;\n"
-	"       pxor 16(%2), %%mm2   ;\n"
-	"       pxor   (%4), %%mm0   ;\n"
-	"       pxor  8(%4), %%mm1   ;\n"
-	"       pxor 16(%3), %%mm2   ;\n"
-	"       movq 24(%1), %%mm3   ;\n"
-	"       pxor   (%5), %%mm0   ;\n"
-	"       pxor  8(%5), %%mm1   ;\n"
-	"       movq %%mm0,   (%1)   ;\n"
-	"       pxor 16(%4), %%mm2   ;\n"
-	"       pxor 24(%2), %%mm3   ;\n"
-	"       movq %%mm1,  8(%1)   ;\n"
-	"       pxor 16(%5), %%mm2   ;\n"
-	"       pxor 24(%3), %%mm3   ;\n"
-	"       movq 32(%1), %%mm4   ;\n"
-	"       movq %%mm2, 16(%1)   ;\n"
-	"       pxor 24(%4), %%mm3   ;\n"
-	"       pxor 32(%2), %%mm4   ;\n"
-	"       movq 40(%1), %%mm5   ;\n"
-	"       pxor 24(%5), %%mm3   ;\n"
-	"       pxor 32(%3), %%mm4   ;\n"
-	"       pxor 40(%2), %%mm5   ;\n"
-	"       movq %%mm3, 24(%1)   ;\n"
-	"       pxor 32(%4), %%mm4   ;\n"
-	"       pxor 40(%3), %%mm5   ;\n"
-	"       movq 48(%1), %%mm6   ;\n"
-	"       movq 56(%1), %%mm7   ;\n"
-	"       pxor 32(%5), %%mm4   ;\n"
-	"       pxor 40(%4), %%mm5   ;\n"
-	"       pxor 48(%2), %%mm6   ;\n"
-	"       pxor 56(%2), %%mm7   ;\n"
-	"       movq %%mm4, 32(%1)   ;\n"
-	"       pxor 48(%3), %%mm6   ;\n"
-	"       pxor 56(%3), %%mm7   ;\n"
-	"       pxor 40(%5), %%mm5   ;\n"
-	"       pxor 48(%4), %%mm6   ;\n"
-	"       pxor 56(%4), %%mm7   ;\n"
-	"       movq %%mm5, 40(%1)   ;\n"
-	"       pxor 48(%5), %%mm6   ;\n"
-	"       pxor 56(%5), %%mm7   ;\n"
-	"       movq %%mm6, 48(%1)   ;\n"
-	"       movq %%mm7, 56(%1)   ;\n"
-      
-	"       addl $64, %1         ;\n"
-	"       addl $64, %2         ;\n"
-	"       addl $64, %3         ;\n"
-	"       addl $64, %4         ;\n"
-	"       addl $64, %5         ;\n"
-	"       decl %0              ;\n"
-	"       jnz 1b               ;\n"
-	"	popl %5\n"
-	"	popl %4\n"
-	: "+g" (lines),
-	  "+r" (p1), "+r" (p2), "+r" (p3)
-	: "r" (p4), "r" (p5)
-	: "memory");
-
+	__asm__ __volatile(".align 32,0x90");
+	do {
+		__asm__ __volatile__ (
+		       "movq   (%0), %%mm0   ;\n"
+		"       movq  8(%0), %%mm1   ;\n"
+		"       pxor   (%1), %%mm0   ;\n"
+		"       pxor  8(%1), %%mm1   ;\n"
+		"       movq 16(%0), %%mm2   ;\n"
+		"       pxor   (%2), %%mm0   ;\n"
+		"       pxor  8(%2), %%mm1   ;\n"
+		"       pxor 16(%1), %%mm2   ;\n"
+		"       pxor   (%3), %%mm0   ;\n"
+		"       pxor  8(%3), %%mm1   ;\n"
+		"       pxor 16(%2), %%mm2   ;\n"
+		"       movq 24(%0), %%mm3   ;\n"
+		"       pxor   (%4), %%mm0   ;\n"
+		"       pxor  8(%4), %%mm1   ;\n"
+		"       movq %%mm0,   (%0)   ;\n"
+		"       pxor 16(%3), %%mm2   ;\n"
+		"       pxor 24(%1), %%mm3   ;\n"
+		"       movq %%mm1,  8(%0)   ;\n"
+		"       pxor 16(%4), %%mm2   ;\n"
+		"       pxor 24(%2), %%mm3   ;\n"
+		"       movq 32(%0), %%mm4   ;\n"
+		"       movq %%mm2, 16(%0)   ;\n"
+		"       pxor 24(%3), %%mm3   ;\n"
+		"       pxor 32(%1), %%mm4   ;\n"
+		"       movq 40(%0), %%mm5   ;\n"
+		"       pxor 24(%4), %%mm3   ;\n"
+		"       pxor 32(%2), %%mm4   ;\n"
+		"       pxor 40(%1), %%mm5   ;\n"
+		"       movq %%mm3, 24(%0)   ;\n"
+		"       pxor 32(%3), %%mm4   ;\n"
+		"       pxor 40(%2), %%mm5   ;\n"
+		"       movq 48(%0), %%mm6   ;\n"
+		"       movq 56(%0), %%mm7   ;\n"
+		"       pxor 32(%4), %%mm4   ;\n"
+		"       pxor 40(%3), %%mm5   ;\n"
+		"       pxor 48(%1), %%mm6   ;\n"
+		"       pxor 56(%1), %%mm7   ;\n"
+		"       movq %%mm4, 32(%0)   ;\n"
+		"       pxor 48(%2), %%mm6   ;\n"
+		"       pxor 56(%2), %%mm7   ;\n"
+		"       pxor 40(%4), %%mm5   ;\n"
+		"       pxor 48(%3), %%mm6   ;\n"
+		"       pxor 56(%3), %%mm7   ;\n"
+		"       movq %%mm5, 40(%0)   ;\n"
+		"       pxor 48(%4), %%mm6   ;\n"
+		"       pxor 56(%4), %%mm7   ;\n"
+		"       movq %%mm6, 48(%0)   ;\n"
+		"       movq %%mm7, 56(%0)   ;\n"
+	      
+		"       addl $64, %0         ;\n"
+		"       addl $64, %1         ;\n"
+		"       addl $64, %2         ;\n"
+		"       addl $64, %3         ;\n"
+		"       addl $64, %4         ;\n"
+		: "+r" (p1), "+r" (p2), "+r" (p3),
+		  "+r" (p4), "+r" (p5)
+		:
+		: "memory");
+	} while (--lines);
+	
 	kernel_fpu_end();
 }
 

Re: raid 5 with >= 5 members broken on x86

[Linus Torvalds]

On Thu, 26 Feb 2004, Linus Torvalds wrote:
> 
> Btw, the "xor_pII_mmx_5()" thing just uses "+r" for the line count, so why 
> doesn't that work for this case?

In other words, shouldn't this work for all compilers also? I don't see 
why this shouldn't compile if the pII version compiles? 

Yes, it's pushing the register pressure a bit, but it would seem to be
the simplest fix..

		Linus

===== include/asm-i386/xor.h 1.14 vs edited =====
--- 1.14/include/asm-i386/xor.h	Tue Mar 11 18:15:03 2003
+++ edited/include/asm-i386/xor.h	Thu Feb 26 14:03:17 2004
@@ -489,7 +489,7 @@
 	"       jnz 1b               ;\n"
 	"	popl %5\n"
 	"	popl %4\n"
-	: "+g" (lines),
+	: "+r" (lines),
 	  "+r" (p1), "+r" (p2), "+r" (p3)
 	: "r" (p4), "r" (p5)
 	: "memory");

Re: raid 5 with >= 5 members broken on x86

[Alexandre Oliva]

On Feb 26, 2004, Linus Torvalds <torvalds@osdl.org> wrote:

> On Thu, 26 Feb 2004, Alexandre Oliva wrote:
>> 
>> I suppose I could just change lines from +g to +r, like xor_pII_mmx_5,
>> but avoiding the pushes and pops is more efficient, and making sure
>> GCC doesn't get clever about sharing or reusing p4 and p5, it's just
>> as safe.  This approach should probably be extended to the other uses
>> of push and pop due to limitations in the number of operands.

> You can't do this in a separate inline asm.

There's a reason why I added both asms, one with volatile and one
without.  I know what I'm doing.  I even tried to explain it in the
comments.  Did you read them?  Let me try again.

+	__asm__ ("" : "+r" (p4), "+r" (p5));

This makes sure GCC no longer knows what's in p4 and p5.  They're no
longer shared with anything they might have been shared with before.
So, when we read from p4 and p5, we know that, even if p4 and p5 are
reloaded into some other register, they're not shared with anything
else.  We don't need the above to be volatile because it's ok to
reorder it with the preparation of the arguments for the asm below, or
with anything before.  The point is only to get rid of any potential
sharing of value that the variables might have with whatever might
have been assigned to them before.

 	__asm__ __volatile__ (
[...]
 	: "+g" (lines),
 	  "+r" (p1), "+r" (p2), "+r" (p3)
 	: "r" (p4), "r" (p5)
 	: "memory");

Ok, so we read from p4 and p5, that GCC knew nothing about.  GCC
doesn't know they changed.  But that's ok, because immediately after
this volatile asm, there's another:

+	__asm__ __volatile__ ("" : "+r" (p4), "+r" (p5));

This one tells GCC: look, I'm clobbering these registers.  They no
longer have the values they used to.  Strictly speaking, this is not
even necessary, since the variables go out of scope at the end of the
function.  But strictly speaking, it's correct, in that it implies GCC
will make no assumptions that the registers that held the values of p4
and p5 at the end of the previous asm statement still do.

So the only possibility of problem would be in case GCC used the
registers with the modified values of p4 and p5 as addresses for
output reloads for any of the other operands of the second asm (the
first volatile one).  But GCC can't possibly do this because it has no
idea of what values p4 and p5 have.

So it the assembly sequence is strictly correct, even though it
requires some deep knowledge of the semantics of asm statements to
conclude that.

> There is nothing to say that gcc wouldn't do a re-load or something
> in between, so you really need to tell the _first_ ask about it.

The only other reload it could do is an input reload of p4 and p5,
which, again, doesn't matter, because p4 and p5 are dead anyway.
Should we actually be interested in their values, I very much agree
with you it wouldn't work.  But in this case, we don't need their
values.  We just want to tell GCC it doesn't know what's in those
variables any more.

So, it doesn't know what's in the p4 and p5 registers before the asm
volatile, because of the first non-volatile asm, and it doesn't know
what's in the variables afterwards, because of the last volatile asm,
so (i) it won't attempt to reuse the values that are modified in the
asm even though it doesn't know, and (ii) these registers won't have
been reused with anything else from before.

I claim it's safe and correct.

>> Yet another possibility is to just use +r for p4 and p5; this works in
>> GCC 3.1 and above.  I wasn't sure the kernel was willing to require
>> that, so I took the most conservative approach.

> No, I don't think we're ready to force a bigger and slower compiler
> on x86 for something like this.

Ok.

> One approach is to just do the loop _outside_ of the asm?

IIUC the loop has to be aligned to work as quickly as possible.

> Btw, the "xor_pII_mmx_5()" thing just uses "+r" for the line count,
> so why doesn't that work for this case?

It does.  I even said so.  But it's slower because of the unnecessary
pushes and pops.  The optimization I propose here could be used for
xor_pII_mmx_5 as well.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Happy GNU Year!                     oliva@{lsd.ic.unicamp.br, gnu.org}
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: raid 5 with >= 5 members broken on x86

[Linus Torvalds]

On Thu, 26 Feb 2004, Alexandre Oliva wrote:
> 
> There's a reason why I added both asms, one with volatile and one
> without.  I know what I'm doing.  I even tried to explain it in the
> comments.  Did you read them?  Let me try again.

Ok, I'll buy it.

> > There is nothing to say that gcc wouldn't do a re-load or something
> > in between, so you really need to tell the _first_ ask about it.
> 
> The only other reload it could do is an input reload of p4 and p5,
> which, again, doesn't matter, because p4 and p5 are dead anyway.

Ok. That's the missing piece. The thing is wrong, but we don't care, 
because even if gcc saves the old values for some silly reload, they're 
dead and uninteresting.

Ok. I did the silly one-liner, but if the "don't care" approach really 
improves code generation, feel free to send one that fixes both the P5 and 
PII cases..

		Linus

Re: raid 5 with >= 5 members broken on x86

[Alexandre Oliva]

On Feb 26, 2004, Linus Torvalds <torvalds@osdl.org> wrote:

>> > There is nothing to say that gcc wouldn't do a re-load or something
>> > in between, so you really need to tell the _first_ ask about it.

>> The only other reload it could do is an input reload of p4 and p5,
>> which, again, doesn't matter, because p4 and p5 are dead anyway.

> Ok. That's the missing piece. The thing is wrong, but we don't care, 
> because even if gcc saves the old values for some silly reload, they're 
> dead and uninteresting.

Yup.

> Ok. I did the silly one-liner

That's good enough for me.  I tested that before trying this better
approach, and it worked.

> but if the "don't care" approach really improves code generation,
> feel free to send one that fixes both the P5 and PII cases..

It's not the code generation that is improved, it's just that we can
then refrain from pushing and popping something that nobody cares
about.  It would have worked to just assign a random value to p4 and
p5 after the asm loop; it would be dead anyway.  As long as we made
sure p4 and p5 weren't shared with anything else before, that is.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Happy GNU Year!                     oliva@{lsd.ic.unicamp.br, gnu.org}
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: raid 5 with >= 5 members broken on x86

[Alexandre Oliva]

On Feb 26, 2004, Linus Torvalds <torvalds@osdl.org> wrote:

> Ok. I did the silly one-liner, but if the "don't care" approach really 
> improves code generation, feel free to send one that fixes both the P5 and 
> PII cases..

FWIW, I think the silly one-liner is actually an improvement, since
then we use a hardware register for the counter, instead of a stack
location.  I was concerned about not increasing the register pressure
with the patch; it looked very tight already, and I couldn't tell it
wouldn't be exceeded with some older compiler that failed to eliminate
the frame pointer, for example.

If that's the way to go, I'll post a patch that leaves the +r alone.
If using a stack location for the counter could possibly be as
efficient as using a register, I'd convert  "+r" (lines) to "+g".  Any
preferences?

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Happy GNU Year!                     oliva@{lsd.ic.unicamp.br, gnu.org}
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: raid 5 with >= 5 members broken on x86

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

On Feb 26, 2004, Linus Torvalds <torvalds@osdl.org> wrote:

> Ok. I did the silly one-liner, but if the "don't care" approach really 
> improves code generation, feel free to send one that fixes both the P5 and 
> PII cases..

Here's an updated patch that is supposed to apply cleanly after the
one-liner you've already checked in.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: i386-xor-stack-optimize.patch --]
[-- Type: text/x-patch, Size: 3591 bytes --]

--- include/asm-i386/xor.h.orig	2004-02-26 19:41:22.000000000 -0300
+++ include/asm-i386/xor.h	2004-02-26 19:48:24.000000000 -0300
@@ -182,11 +182,15 @@
 
 	kernel_fpu_begin();
 
-	/* need to save/restore p4/p5 manually otherwise gcc's 10 argument
-	   limit gets exceeded (+ counts as two arguments) */
+	/* Make sure GCC forgets anything it knows about p4 or p5,
+	   such that it won't pass to the asm volatile below a
+	   register that is shared with any other variable.  That's
+	   because we modify p4 and p5 there, but we can't mark them
+	   as read/write, otherwise we'd overflow the 10-asm-operands
+	   limit of GCC < 3.1.  */
+	__asm__ ("" : "+r" (p4), "+r" (p5));
+
 	__asm__ __volatile__ (
-		"  pushl %4\n"
-		"  pushl %5\n"
 #undef BLOCK
 #define BLOCK(i) \
 	LD(i,0)					\
@@ -229,13 +233,16 @@
 	"       addl $128, %5         ;\n"
 	"       decl %0               ;\n"
 	"       jnz 1b                ;\n"
-	"	popl %5\n"
-	"	popl %4\n"
 	: "+r" (lines),
 	  "+r" (p1), "+r" (p2), "+r" (p3)
 	: "r" (p4), "r" (p5) 
 	: "memory");
 
+	/* p4 and p5 were modified, and now the variables are dead.
+	   Clobber them just to be sure nobody does something stupid
+	   like assuming they have some legal value.  */
+	__asm__ ("" : "=r" (p4), "=r" (p5));
+
 	kernel_fpu_end();
 }
 
@@ -425,10 +432,15 @@
 
 	kernel_fpu_begin();
 
-	/* need to save p4/p5 manually to not exceed gcc's 10 argument limit */
+	/* Make sure GCC forgets anything it knows about p4 or p5,
+	   such that it won't pass to the asm volatile below a
+	   register that is shared with any other variable.  That's
+	   because we modify p4 and p5 there, but we can't mark them
+	   as read/write, otherwise we'd overflow the 10-asm-operands
+	   limit of GCC < 3.1.  */
+	__asm__ ("" : "+r" (p4), "+r" (p5));
+
 	__asm__ __volatile__ (
-	"	pushl %4\n"
-	"	pushl %5\n"        	
 	" .align 32,0x90             ;\n"
 	" 1:                         ;\n"
 	"       movq   (%1), %%mm0   ;\n"
@@ -487,13 +499,16 @@
 	"       addl $64, %5         ;\n"
 	"       decl %0              ;\n"
 	"       jnz 1b               ;\n"
-	"	popl %5\n"
-	"	popl %4\n"
 	: "+r" (lines),
 	  "+r" (p1), "+r" (p2), "+r" (p3)
 	: "r" (p4), "r" (p5)
 	: "memory");
 
+	/* p4 and p5 were modified, and now the variables are dead.
+	   Clobber them just to be sure nobody does something stupid
+	   like assuming they have some legal value.  */
+	__asm__ ("" : "=r" (p4), "=r" (p5));
+
 	kernel_fpu_end();
 }
 
@@ -757,10 +772,15 @@
 
 	XMMS_SAVE;
 
-	/* need to save p4/p5 manually to not exceed gcc's 10 argument limit */
+	/* Make sure GCC forgets anything it knows about p4 or p5,
+	   such that it won't pass to the asm volatile below a
+	   register that is shared with any other variable.  That's
+	   because we modify p4 and p5 there, but we can't mark them
+	   as read/write, otherwise we'd overflow the 10-asm-operands
+	   limit of GCC < 3.1.  */
+	__asm__ ("" : "+r" (p4), "+r" (p5));
+
         __asm__ __volatile__ (
-		" pushl %4\n"
-		" pushl %5\n"
 #undef BLOCK
 #define BLOCK(i) \
 		PF1(i)					\
@@ -817,13 +837,16 @@
         "       addl $256, %5           ;\n"
         "       decl %0                 ;\n"
         "       jnz 1b                  ;\n"
-	"	popl %5\n"	
-	"	popl %4\n"	
 	: "+r" (lines),
 	  "+r" (p1), "+r" (p2), "+r" (p3)
 	: "r" (p4), "r" (p5)
 	: "memory");
 
+	/* p4 and p5 were modified, and now the variables are dead.
+	   Clobber them just to be sure nobody does something stupid
+	   like assuming they have some legal value.  */
+	__asm__ ("" : "=r" (p4), "=r" (p5));
+
 	XMMS_RESTORE;
 }
 

[-- Attachment #3: Type: text/plain, Size: 289 bytes --]


-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Happy GNU Year!                     oliva@{lsd.ic.unicamp.br, gnu.org}
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist                Professional serial bug killer