Subject: apparent math-emu hang on movf instruction

From: [email-masked]
Date: Wed, 28 Jul 2004 12:54:04 -0700

I hope this isn't a FAQ or a complete newbie oversight, but I haven't
found any mention of a similar problem in the FAQs or in the last 18
months' archives of this list.

I have some floating-point code that hangs, apparently on a movf
instruction, with CPU usage pegged at 99%.  It's interruptible, but
that's the best that can be said about it.

The C code in question is a simple comparison of an integer array
element with a floating-point constant (0.0, in fact).  When (cross-)
compiled with either gcc 3.3.4 or 3.3.1 and -O1 (or -O2) and -mtune=r4600
-mips32, it generates the movf instruction.  Compiled either without
optimization or without the r4600/mips32 options, it generates a branch
instead; that version works.  My attempts so far to create a simplified
test case have not succeeded; it seems to be slightly tricky to get GCC
to emit this instruction.  (I'm not an expert in either GCC, the Linux
kernel, or MIPS hardware, btw, so apologies if I'm completely off-track
here.)

The kernel is question is a 2.4.20 derivative (MontaVista Linux 3.1),
and the processor is a Philips PR4450:  MIPS32 core, no floating-point,
some minor extensions and timing differences in a few instructions.
We're using the kernel's FP emulation rather than GCC's soft-float,
and my working assumption at this point is that it's a problem with
the emulation, since movf is described as "move conditional on floating-
point false."

	Q1:  Is that a correct assumption?

I've just started looking at the kernel's emulation code, and one thing
that pops out immediately is that there's no mention of movf anywhere
in arch/mips/math-emu, though cp1emu.c does cover movc, movn, movz, and
plain mov (at least, their .fmt variants).  Nor does the latest(?) CVS
version (http://www.linux-mips.org/cvsweb/linux/arch/mips/math-emu/
cp1emu.c?rev=1.32&content-type=text/x-cvsweb-markup).  On the other hand,
they also don't mention bc1t, "branch on FP true"--which works--so I'm
probably not on the right track here.

	Q2:  Am I?  I would have expected an unemulated instruction to
	     throw an illegal instruction exception or cause an oops or
	     something, not to spin indefinitely.

	Q3:  Any suggestions for what to test/look at/do next?

(Of course, it's also possible that it's a hardware problem of some sort,
although it doesn't feel that way to me.  Then again, I don't trust such
feelings very far...)

Here's the C code that fails:

	if (a[m][n] != 0.0)
	    i++;

Here's its assembler output ($f20 was previously loaded with zero via
"mtc1 $0,$f20", and $16 contains the pointer a[m]):

        l.s     $f2,0($16)      # $f2 = a[m][n]  (32-bit)
        lw      $3,24($sp)      # $3 = i
        cvt.d.w $f0,$f2         # FP: convert int32 ($f2) to double ($f0)
        c.eq.d  $f0,$f20        # FP: conditional:  $f0 == $f20 ?
        addu    $2,$3,1         # add unsigned:  $2 = i + 1
        la      $4,$LC32        # "DEBUG:  checking ..." (later printf)
        movf    $3,$2,$fcc0     # move if FP false (fcc0 = 0):  $3 = $2 = i+1
        sw      $3,24($sp)      # i = $3 (store on stack)
	 ... [printf stuff, etc.]

Thanks for any help/suggestions,
-- 
Greg Roelofs            `Name an animal that's small and fuzzy.' `Mold.'
Philips Semiconductors   [email-masked]

---

From: Richard Sandiford <[email-masked]>
Date: Thu, 29 Jul 2004 07:54:50 +0100

[email-masked] writes:
> plain mov (at least, their .fmt variants).

"movc" covers both movf and movt (bit 16 says which).

But yes, it does look like an emulation bug.  It seems that the code
to handle GPR conditional moves (in cop1Emulate) was copied from the
code to handle FPR moves (in fpu_emu).  The cop1Emulate() code reads:

		if (((ctx->fcr31 & cond) != 0) != ((MIPSInst_RT(ir) & 1) != 0))
			return 0;
		xcp->regs[MIPSInst_RD(ir)] = xcp->regs[MIPSInst_RS(ir)];

"return 0" was fine in fpu_emu(), but not here, since it skips the
all-important:

	/* we did it !! */
	xcp->cp0_epc = VA_TO_REG(contpc);
	xcp->cp0_cause &= ~CAUSEF_BD;
	return 0;

So we'd reenter the exception handler on exit.

Does the patch below (against 2.6) fix things?  Only the first hunk
is needed to fix the bug, the rest is just there for consistency.

Richard

Index: arch/mips/math-emu/cp1emu.c
===================================================================
RCS file: /home/cvs/linux/arch/mips/math-emu/cp1emu.c,v
retrieving revision 1.32
diff -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.32 cp1emu.c
--- arch/mips/math-emu/cp1emu.c	19 Jan 2004 16:25:21 -0000	1.32
+++ arch/mips/math-emu/cp1emu.c	29 Jul 2004 06:42:53 -0000
@@ -528,9 +528,8 @@ static int cop1Emulate(struct pt_regs *x
 		if (MIPSInst_FUNC(ir) != movc_op)
 			return SIGILL;
 		cond = fpucondbit[MIPSInst_RT(ir) >> 2];
-		if (((ctx->fcr31 & cond) != 0) != ((MIPSInst_RT(ir) & 1) != 0))
-			return 0;
-		xcp->regs[MIPSInst_RD(ir)] = xcp->regs[MIPSInst_RS(ir)];
+		if (((ctx->fcr31 & cond) != 0) == ((MIPSInst_RT(ir) & 1) != 0))
+			xcp->regs[MIPSInst_RD(ir)] = xcp->regs[MIPSInst_RS(ir)];
 		break;
 #endif
 
@@ -850,20 +849,17 @@ static int fpu_emu(struct pt_regs *xcp, 
 #if __mips >= 4
 		case fmovc_op:
 			cond = fpucondbit[MIPSInst_FT(ir) >> 2];
-			if (((ctx->fcr31 & cond) != 0) !=
+			if (((ctx->fcr31 & cond) != 0) ==
 				((MIPSInst_FT(ir) & 1) != 0))
-				return 0;
-			SPFROMREG(rv.s, MIPSInst_FS(ir));
+				SPFROMREG(rv.s, MIPSInst_FS(ir));
 			break;
 		case fmovz_op:
-			if (xcp->regs[MIPSInst_FT(ir)] != 0)
-				return 0;
-			SPFROMREG(rv.s, MIPSInst_FS(ir));
+			if (xcp->regs[MIPSInst_FT(ir)] == 0)
+				SPFROMREG(rv.s, MIPSInst_FS(ir));
 			break;
 		case fmovn_op:
-			if (xcp->regs[MIPSInst_FT(ir)] == 0)
-				return 0;
-			SPFROMREG(rv.s, MIPSInst_FS(ir));
+			if (xcp->regs[MIPSInst_FT(ir)] != 0)
+				SPFROMREG(rv.s, MIPSInst_FS(ir));
 			break;
 #endif
 		case fabs_op:
@@ -1040,20 +1036,17 @@ static int fpu_emu(struct pt_regs *xcp, 
 #if __mips >= 4
 		case fmovc_op:
 			cond = fpucondbit[MIPSInst_FT(ir) >> 2];
-			if (((ctx->fcr31 & cond) != 0) !=
+			if (((ctx->fcr31 & cond) != 0) ==
 				((MIPSInst_FT(ir) & 1) != 0))
-				return 0;
-			DPFROMREG(rv.d, MIPSInst_FS(ir));
+				DPFROMREG(rv.d, MIPSInst_FS(ir));
 			break;
 		case fmovz_op:
-			if (xcp->regs[MIPSInst_FT(ir)] != 0)
-				return 0;
-			DPFROMREG(rv.d, MIPSInst_FS(ir));
+			if (xcp->regs[MIPSInst_FT(ir)] == 0)
+				DPFROMREG(rv.d, MIPSInst_FS(ir));
 			break;
 		case fmovn_op:
-			if (xcp->regs[MIPSInst_FT(ir)] == 0)
-				return 0;
-			DPFROMREG(rv.d, MIPSInst_FS(ir));
+			if (xcp->regs[MIPSInst_FT(ir)] != 0)
+				DPFROMREG(rv.d, MIPSInst_FS(ir));
 			break;
 #endif
 		case fabs_op:

---

From: Richard Sandiford <[email-masked]>
Date: Thu, 29 Jul 2004 08:01:24 +0100

> is needed to fix the bug, the rest is just there for consistency.

Oops!  Serves me right for dabbling in new code.  Only the first
hunk is correct.

Richard

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Fri, 30 Jul 2004 11:48:31 +0200 (CEST)

> Oops!  Serves me right for dabbling in new code.  Only the first

 Thanks Richard for working on this.  I've verified your patch on my
system and as it is obviously correct, I've applied it with a trivial
formatting change to our CVS, both to the head and to the 2.4 branch; with
appropriate literal name changes for the latter.

  Maciej