Re: V4L2: __ucmpdi2 undefined on ppc

On Thu, 2006-12-14 at 14:58 -0500, Kyle McMartin wrote:

I posted a patch to Paul this week to fix this, 

Hm, I didn't see it on linuxppc-dev.

Since ppc32 can't do a 64bit comparison on its own it seems, gcc
will generate a call to a helper function from libgcc. What other
arches do is link libgcc.a into libs-y, and export the symbol
they want from it.

You still get to 'accidentally' do 64-bit arithmetic in-kernel that way
though. Might be better just to provide __ucmpdi2, just as we have for
the other functions which are required from libgcc 

It'd be better just to fix the compiler though -- which is in fact what
they've done: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25724
              http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21237

I've applied this as a temporary hack to the Fedora kernel until the
compiler is updated there...

--- linux-2.6.19.ppc/arch/powerpc/kernel/misc_32.S~	2006-11-29 21:57:37.000000000 +0000
+++ linux-2.6.19.ppc/arch/powerpc/kernel/misc_32.S	2006-12-17 12:19:48.000000000 +0000

@@ -728,6 +728,27 @@ _GLOBAL(__lshrdi3)
 	or	r4,r4,r7	# LSW |= t2
 	blr
 
+/*
+ * __ucmpdi2: 64-bit comparison
+ *
+ * R3/R4 has 64 bit value A
+ * R5/R6 has 64 bit value B
+ * result in R3: 0 for A < B
+ *		 1 for A == B
+ *		 2 for A > B
+ */
+_GLOBAL(__ucmpdi2)
+	cmplw	r7,r3,r5	# compare high words
+	li	r3,0
+	blt	r7,2f		# a < b ... return 0
+	bgt	r7,1f		# a > b ... return 2
+	cmplw	r6,r4,r6	# compare low words
+	blt	r6,2f		# a < b ... return 0
+	li	r3,1
+	ble	r6,2f		# a = b ... return 1
+1:	li	r3,2
+2:	blr
+
 _GLOBAL(abs)
 	srawi	r4,r3,31
 	xor	r3,r3,r4
--- linux-2.6.19.ppc/arch/powerpc/kernel/ppc_ksyms.c~	2006-12-15 17:19:56.000000000 +0000
+++ linux-2.6.19.ppc/arch/powerpc/kernel/ppc_ksyms.c	2006-12-17 12:16:54.000000000 +0000

@@ -161,9 +161,11 @@ EXPORT_SYMBOL(to_tm);
 long long __ashrdi3(long long, int);
 long long __ashldi3(long long, int);
 long long __lshrdi3(long long, int);
+int __ucmpdi2(uint64_t, uint64_t);
 EXPORT_SYMBOL(__ashrdi3);
 EXPORT_SYMBOL(__ashldi3);
 EXPORT_SYMBOL(__lshrdi3);
+EXPORT_SYMBOL(__ucmpdi2);
 #endif
 
 EXPORT_SYMBOL(memcpy);

-- 
dwmw2

On 12/17/06, David Woodhouse [off-list ref] wrote:

You still get to 'accidentally' do 64-bit arithmetic in-kernel that way
though. Might be better just to provide __ucmpdi2, just as we have for
the other functions which are required from libgcc

It'd be better just to fix the compiler though -- which is in fact what
they've done: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25724
              http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21237

I've applied this as a temporary hack to the Fedora kernel until the
compiler is updated there...

Hello,

We're hitting this i nouveau as well (http://nouveau.freedesktop.org),
since we make extensive use ot 64 bit ints. Over time, we've had a
number of reports on this issue, and at one point I read that it
should be fixed in gcc. But recently, a nouveau user on PPC32 (Henrik
in CC:) reported the issue again with gcc 4.2.3. Others have it on gcc
4.2.2 too:
http://bugs.freedesktop.org/show_bug.cgi?id=10547

So, the point of this email is to ask about the possibility of merging
in one of the __ucmpdi2 patches, like David's which is kept below for
reference. Most distros seem to ship with such a patch already, and it
seems that other drivers hit this as well.

Thanks,
Stephane

On 2/6/08, Stephane Marchesin [off-list ref] wrote:

 We're hitting this i nouveau as well (http://nouveau.freedesktop.org),
 since we make extensive use ot 64 bit ints. Over time, we've had a
 number of reports on this issue, and at one point I read that it
 should be fixed in gcc. But recently, a nouveau user on PPC32 (Henrik
 in CC:) reported the issue again with gcc 4.2.3. Others have it on gcc
 4.2.2 too:
 http://bugs.freedesktop.org/show_bug.cgi?id=10547

 So, the point of this email is to ask about the possibility of merging
 in one of the __ucmpdi2 patches, like David's which is kept below for
 reference. Most distros seem to ship with such a patch already, and it
 seems that other drivers hit this as well.

So, could we have that thing in main tree ? It's not like it's
untested, most distros carry that, and a couple of arches provide
their own ucmpdi2 implementation already. It's also such a small
function...

Stephane

quoted

+/*
+ * __ucmpdi2: 64-bit comparison
+ *
+ * R3/R4 has 64 bit value A
+ * R5/R6 has 64 bit value B
+ * result in R3: 0 for A < B
+ *              1 for A == B
+ *              2 for A > B
+ */
+_GLOBAL(__ucmpdi2)
+       cmplw   r7,r3,r5        # compare high words
+       li      r3,0
+       blt     r7,2f           # a < b ... return 0
+       bgt     r7,1f           # a > b ... return 2
+       cmplw   r6,r4,r6        # compare low words
+       blt     r6,2f           # a < b ... return 0
+       li      r3,1
+       ble     r6,2f           # a = b ... return 1
+1:     li      r3,2
+2:     blr

Every occurrence of r7 here is wrong (and some of the r6).  Is there
any reason to do this in assembler code at all?


Segher

On Sun, 2008-03-02 at 22:53 +0100, Segher Boessenkool wrote:

Every occurrence of r7 here is wrong (and some of the r6).

Can you elucidate?

  Is there any reason to do this in assembler code at all?

Is there any particular reason not to?

-- 
dwmw2

On Tue, Mar 04, 2008 at 04:37:28PM +0000, David Woodhouse wrote:

On Sun, 2008-03-02 at 22:53 +0100, Segher Boessenkool wrote:

quoted

Every occurrence of r7 here is wrong (and some of the r6).

Can you elucidate?

They were being used as condition registers rather than GPRs.

quoted

  Is there any reason to do this in assembler code at all?

Is there any particular reason not to?

Same reason most of the rest of the kernel is in C rather than assembly.

Of course, a better question is why we don't just link libgcc...

-Scott

quoted

Every occurrence of r7 here is wrong (and some of the r6).

Can you elucidate?

Sure!  It should read either "7" or "cr7".

quoted

  Is there any reason to do this in assembler code at all?

Is there any particular reason not to?

1) If written in assembler, it needs to be written for every 
architecture
separately, and sometimes even for every architecture variant (32 vs. 64
bit is only the tip of the iceberg).

2) As shown here as well as in the recent strncmp() patch, it is a lot 
harder
to write correct assembler code than it is to write correct C code(*), 
and
the C code isn't less efficient either.


Segher

(*) Well, the generic C strncmp() code in the kernel is broken too, bad
example perhaps :-)

Segher Boessenkool writes:

Every occurrence of r7 here is wrong (and some of the r6).  Is there
any reason to do this in assembler code at all?

The fact that the obvious C code generates ... a call to __ucmpdi2? :)

Paul.

Paul Mackerras wrote:

Segher Boessenkool writes:

quoted

Every occurrence of r7 here is wrong (and some of the r6).  Is there
any reason to do this in assembler code at all?

The fact that the obvious C code generates ... a call to __ucmpdi2? :)

So use the correct C code, not the obvious C code. :-)

-Scott

quoted

Every occurrence of r7 here is wrong (and some of the r6).  Is there
any reason to do this in assembler code at all?

The fact that the obvious C code generates ... a call to __ucmpdi2? :)

Hrm?  Here's the "obvious" C code, portable to all architectures
(modulo the specific types used, this isn't a proposed patch):


unsigned int ucmpdi2(unsigned long long a, unsigned long long b)
{
         unsigned long ahi, bhi, alo, blo;

         ahi = a >> 32;
         bhi = b >> 32;
         if (ahi < bhi)
                 return 0;
         if (ahi > bhi)
                 return 2;

         alo = a;
         blo = b;
         if (alo < blo)
                 return 0;
         if (alo > blo)
                 return 2;

         return 1;
}


(libgcc does it a bit differently, with unions and stuff).


Segher