[PATCH] aarch64 sim fcsel bug fix

[PATCH] aarch64 sim fcsel bug fix

[Jim Wilson]

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

The fcsel instruction is storing source register numbers in the
destination register, instead of source register contents.  There are
missing calls to fetch the contents of the source registers.

While looking at this, I ran into the problem that when an FP register
changes from plus zero to minus zero, or vice versa, I don't get any
output with --trace-register.  The GCC C testcase I was looking at
happened to be testing support for signed zeros, and the source
register number happened to be zero, so I needed this to work right to
see what was going wrong.  I added signbit calls to catch this case.

The testcase fails without the patch, and works with the patch.  The
GCC C testsuite unexpected failures drop from 2473 to 2416.

Jim

[-- Attachment #2: aarch64-sim-fcsel.patch --]
[-- Type: text/x-patch, Size: 3003 bytes --]

	sim/aarch64/
	* cpustate.c: Include math.h.
	(aarch64_set_FP_float): Use signbit to check for signed zero.
	(aarch64_set_FP_double): Likewise.
	* simulator.c (dexSimpleFPCondSelect): Call aarch64_get_FP_double or
	aarch64_get_FP_float to get source register contents.

	sim/testsuite/sim/aarch64/
	* fcsel.s: New.

diff --git a/sim/aarch64/cpustate.c b/sim/aarch64/cpustate.c
index 648221f..4b201a7 100644
--- a/sim/aarch64/cpustate.c
+++ b/sim/aarch64/cpustate.c
@@ -20,6 +20,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <stdio.h>
+#include <math.h>
 
 #include "sim-main.h"
 #include "cpustate.h"
@@ -369,7 +370,9 @@ aarch64_set_FP_half (sim_cpu *cpu, VReg reg, float val)
 void
 aarch64_set_FP_float (sim_cpu *cpu, VReg reg, float val)
 {
-  if (val != cpu->fr[reg].s)
+  if (val != cpu->fr[reg].s
+      /* Handle +/- zero.  */
+      || signbit (val) != signbit (cpu->fr[reg].s))
     {
       FRegister v;
 
@@ -385,7 +388,9 @@ aarch64_set_FP_float (sim_cpu *cpu, VReg reg, float val)
 void
 aarch64_set_FP_double (sim_cpu *cpu, VReg reg, double val)
 {
-  if (val != cpu->fr[reg].d)
+  if (val != cpu->fr[reg].d
+      /* Handle +/- zero.  */
+      || signbit (val) != signbit (cpu->fr[reg].d))
     {
       FRegister v;
 
diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index be3d6c7..3f56177 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -7463,9 +7463,11 @@ dexSimpleFPCondSelect (sim_cpu *cpu)
 
   TRACE_DECODE (cpu, "emulated at line %d", __LINE__);
   if (INSTR (22, 22))
-    aarch64_set_FP_double (cpu, sd, set ? sn : sm);
+    aarch64_set_FP_double (cpu, sd, (set ? aarch64_get_FP_double (cpu, sn)
+				     : aarch64_get_FP_double (cpu, sm)));
   else
-    aarch64_set_FP_float (cpu, sd, set ? sn : sm);
+    aarch64_set_FP_float (cpu, sd, (set ? aarch64_get_FP_float (cpu, sn)
+				    : aarch64_get_FP_float (cpu, sm)));
 }
 
 /* Store 32 bit unscaled signed 9 bit.  */
diff --git a/sim/testsuite/sim/aarch64/fcsel.s b/sim/testsuite/sim/aarch64/fcsel.s
new file mode 100644
index 0000000..5b8443c
--- /dev/null
+++ b/sim/testsuite/sim/aarch64/fcsel.s
@@ -0,0 +1,53 @@
+# mach: aarch64
+
+# Check the FP Conditional Select instruction: fcsel.
+# Check 1/1 eq/neg, and 1/2 lt/gt.
+
+.include "testutils.inc"
+
+	start
+	fmov s0, #1.0
+	fmov s1, #1.0
+	fmov s2, #-1.0
+	fcmp s0, s1
+	fcsel s3, s0, s2, eq
+	fcmp s3, s0
+	bne .Lfailure
+	fcsel s3, s0, s2, ne
+	fcmp s3, s2
+	bne .Lfailure
+
+	fmov s0, #1.0
+	fmov s1, #2.0
+	fcmp s0, s1
+	fcsel s3, s0, s2, lt
+	fcmp s3, s0
+	bne .Lfailure
+	fcsel s3, s0, s2, gt
+	fcmp s3, s2
+	bne .Lfailure
+
+	fmov d0, #1.0
+	fmov d1, #1.0
+	fmov d2, #-1.0
+	fcmp d0, d1
+	fcsel d3, d0, d2, eq
+	fcmp d3, d0
+	bne .Lfailure
+	fcsel d3, d0, d2, ne
+	fcmp d3, d2
+	bne .Lfailure
+
+	fmov d0, #1.0
+	fmov d1, #2.0
+	fcmp d0, d1
+	fcsel d3, d0, d2, lt
+	fcmp d3, d0
+	bne .Lfailure
+	fcsel d3, d0, d2, gt
+	fcmp d3, d2
+	bne .Lfailure
+
+	pass
+.Lfailure:
+	fail

Re: [PATCH] aarch64 sim fcsel bug fix

[Nick Clifton]

Hi Jim,

> The fcsel instruction is storing source register numbers in the
> destination register, instead of source register contents.  There are
> missing calls to fetch the contents of the source registers.
> 
> While looking at this, I ran into the problem that when an FP register
> changes from plus zero to minus zero, or vice versa, I don't get any
> output with --trace-register.  The GCC C testcase I was looking at
> happened to be testing support for signed zeros, and the source
> register number happened to be zero, so I needed this to work right to
> see what was going wrong.  I added signbit calls to catch this case.
> 
> The testcase fails without the patch, and works with the patch.  The
> GCC C testsuite unexpected failures drop from 2473 to 2416.

Approved - please apply - and thanks for all of these patches!

Cheers
  Nick