[Qemu-devel] [PATCH] SPARC target : Fix carry flag update in addxcc and subxcc ops

[Qemu-devel] [PATCH] SPARC target : Fix carry flag update in addxcc and subxcc ops

[Even Rouault]

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

I send a patch that should fix a bug in the update of carry flag for addxcc 
and subxcc instructions when the carry flag is set before the evaluation of 
the instruction.
(the fix is identical to what is done in the similar instruction 
op_adcl_T0_T1_cc for arm target)

[-- Attachment #2: qemu-sparc-carry-xcc-ops.patch --]
[-- Type: text/x-diff, Size: 2130 bytes --]

? patch-qemu-sparc-xcc_ops.txt
Index: op.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op.c,v
retrieving revision 1.18
diff -u -p -r1.18 op.c
--- op.c	30 Oct 2005 17:28:50 -0000	1.18
+++ op.c	7 Apr 2006 22:04:40 -0000
@@ -415,9 +415,9 @@ void OPPROTO op_addx_T1_T0(void)
 void OPPROTO op_addx_T1_T0_cc(void)
 {
     target_ulong src1;
-
+    target_ulong has_carry = FLAG_SET(PSR_CARRY);
     src1 = T0;
-    T0 += T1 + FLAG_SET(PSR_CARRY);
+    T0 += T1 + has_carry;
     env->psr = 0;
 #ifdef TARGET_SPARC64
     if (!(T0 & 0xffffffff))
@@ -435,7 +435,7 @@ void OPPROTO op_addx_T1_T0_cc(void)
 	env->xcc |= PSR_ZERO;
     if ((int64_t) T0 < 0)
 	env->xcc |= PSR_NEG;
-    if (T0 < src1)
+    if (T0 < src1 || (has_carry && T0 <= src1))
 	env->xcc |= PSR_CARRY;
     if (((src1 ^ T1 ^ -1) & (src1 ^ T0)) & (1ULL << 63))
 	env->xcc |= PSR_OVF;
@@ -444,7 +444,7 @@ void OPPROTO op_addx_T1_T0_cc(void)
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if (T0 < src1)
+    if (T0 < src1 || (has_carry && T0 <= src1))
 	env->psr |= PSR_CARRY;
     if (((src1 ^ T1 ^ -1) & (src1 ^ T0)) & (1 << 31))
 	env->psr |= PSR_OVF;
@@ -505,9 +505,9 @@ void OPPROTO op_subx_T1_T0(void)
 void OPPROTO op_subx_T1_T0_cc(void)
 {
     target_ulong src1;
-
+    target_ulong has_carry = FLAG_SET(PSR_CARRY);
     src1 = T0;
-    T0 -= T1 + FLAG_SET(PSR_CARRY);
+    T0 -= T1 + has_carry;
     env->psr = 0;
 #ifdef TARGET_SPARC64
     if (!(T0 & 0xffffffff))
@@ -525,7 +525,7 @@ void OPPROTO op_subx_T1_T0_cc(void)
 	env->xcc |= PSR_ZERO;
     if ((int64_t) T0 < 0)
 	env->xcc |= PSR_NEG;
-    if (src1 < T1)
+    if (src1 < T1 || (has_carry && src1 <= T1))
 	env->xcc |= PSR_CARRY;
     if (((src1 ^ T1) & (src1 ^ T0)) & (1ULL << 63))
 	env->xcc |= PSR_OVF;
@@ -534,7 +534,7 @@ void OPPROTO op_subx_T1_T0_cc(void)
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if (src1 < T1)
+    if (src1 < T1 || (has_carry && src1 <= T1))
 	env->psr |= PSR_CARRY;
     if (((src1 ^ T1) & (src1 ^ T0)) & (1 << 31))
 	env->psr |= PSR_OVF;

Re: [Qemu-devel] [PATCH] SPARC target : Fix carry flag update in addxcc and subxcc ops

[Paul Brook]

On Saturday 08 April 2006 14:46, Even Rouault wrote:
> I send a patch that should fix a bug in the update of carry flag for addxcc
> and subxcc instructions when the carry flag is set before the evaluation of
> the instruction.
> (the fix is identical to what is done in the similar instruction
> op_adcl_T0_T1_cc for arm target)

    if ((T0 & 0xffffffff) < (src1 & 0xffffffff))
        env->psr |= PSR_CARRY;

Doesn't this condition in addx and the corresponding line in subx need similar 
treatment?

Paul

Re: [Qemu-devel] [PATCH] SPARC target : Fix carry flag update inaddxcc and subxc

[Blue Swirl]

>Doesn't this condition in addx and the corresponding line in subx need 
>similar
>treatment?

They don't change the flags.

_________________________________________________________________
FREE pop-up blocking with the new MSN Toolbar - get it now! 
http://toolbar.msn.click-url.com/go/onm00200415ave/direct/01/

Re: [Qemu-devel] [PATCH] SPARC target : Fix carry flag update inaddxcc and subxc

[Paul Brook]

On Monday 10 April 2006 19:04, Blue Swirl wrote:
> >Doesn't this condition in addx and the corresponding line in subx need
> >similar
> >treatment?
>
> They don't change the flags.

It certainly look like they do.

In particular if TARGET_SPARC64 we have

    if ((T0 & 0xffffffff) < (src1 & 0xffffffff))
        env->psr |= PSR_CARRY;

But if !TARGET_SPARC64 we have

-    if (T0 < src1)
+    if (T0 < src1 || (has_carry && T0 <= src1))
 	env->psr |= PSR_CARRY;

I'd expect these two to be the same (modulo the explicit truncation in the 
first case).

Paul

Re: [Qemu-devel] [PATCH] SPARC target : Fix carry flag update inaddxcc and subxc

[Laurent Desnogues]

Blue Swirl a écrit :
>> Doesn't this condition in addx and the corresponding line in subx need 
>> similar
>> treatment?
> 
> They don't change the flags.

ADDX (now named ADDC in v9) doesn't change the flags, while ADDXcc
(ADDCcc) does.  The cc versions also compute overflow.

cf. SPARC v9 on www.sparg.org


			Laurent

Re: [Qemu-devel] [PATCH] SPARC target : Fix carry flag update inaddxcc and subxc

[Even Rouault]

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

To be clear, I just ran into the problem of the update of the carry flag but 
I'm not at all a specialist of SPARC assembly.

From what I've understood, Paul is certainly right when suggesting to add a 
fix for the SPARC64 case (for the addxcc and subxcc, not addx and subx) and I 
send a new version of the patch (qemu-sparc-carry-xcc-2.patch)
I've also tried to take into account Blue Swirl's suggestion of storing the 
intermediate results of the addition T0 + FLAG_SET(PSR_CARRY), but the result 
wasn't correct in all cases (specifically if T0=0xFFFFFFFF). I think we 
really need a '<' and '<=' comparison. 
However, I've tried to refactor a bit the code to save some comparisons in 
another version of the patch (qemu-sparc-carry-xcc-3.patch), really close to 
the arm code, and hopefully equivalent to qemu-sparc-carry-xcc-2.patch (but 
less sexy).

As far as the V flag is concerned, mmm, I'm not really sure whether we should 
change something in the sparc code. If we compare to the arm code, we don't 
take into account the fact that the carry flag is set before.

We'd probably need some extensive tests and their associated expected results.

Even

Le Lundi 10 Avril 2006 20:46, Laurent Desnogues a écrit :
> Blue Swirl a écrit :
> >> Doesn't this condition in addx and the corresponding line in subx need
> >> similar
> >> treatment?
> >
> > They don't change the flags.
>
> ADDX (now named ADDC in v9) doesn't change the flags, while ADDXcc
> (ADDCcc) does.  The cc versions also compute overflow.
>
> cf. SPARC v9 on www.sparg.org
>
>
> 			Laurent
>
>
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel

[-- Attachment #2: qemu-sparc-carry-xcc-ops-3.patch --]
[-- Type: text/x-diff, Size: 3724 bytes --]

? qemu-sparc-carry-xcc-ops-2.patch
? qemu-sparc-carry-xcc-ops-3.patch
? qemu-sparc-carry-xcc-ops.patch
Index: op.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op.c,v
retrieving revision 1.18
diff -u -p -r1.18 op.c
--- op.c	30 Oct 2005 17:28:50 -0000	1.18
+++ op.c	10 Apr 2006 20:03:37 -0000
@@ -415,28 +415,50 @@ void OPPROTO op_addx_T1_T0(void)
 void OPPROTO op_addx_T1_T0_cc(void)
 {
     target_ulong src1;
-
     src1 = T0;
-    T0 += T1 + FLAG_SET(PSR_CARRY);
-    env->psr = 0;
+    if (FLAG_SET(PSR_CARRY))
+    {
+      T0 += T1 + 1;
+      env->psr = 0;
+#ifdef TARGET_SPARC64
+      if ((T0 & 0xffffffff) <= (src1 & 0xffffffff))
+        env->psr |= PSR_CARRY;
+      env->xcc = 0;
+      if (T0 <= src1)
+        env->xcc |= PSR_CARRY;
+#else
+      if (T0 <= src1)
+        env->psr |= PSR_CARRY;
+#endif
+    }
+    else
+    {
+      T0 += T1;
+      env->psr = 0;
+#ifdef TARGET_SPARC64
+      if ((T0 & 0xffffffff) < (src1 & 0xffffffff))
+        env->psr |= PSR_CARRY;
+      env->xcc = 0;
+      if (T0 < src1)
+        env->xcc |= PSR_CARRY;
+#else
+      if (T0 < src1)
+        env->psr |= PSR_CARRY;
+#endif
+    }
 #ifdef TARGET_SPARC64
     if (!(T0 & 0xffffffff))
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if ((T0 & 0xffffffff) < (src1 & 0xffffffff))
-	env->psr |= PSR_CARRY;
     if ((((src1 & 0xffffffff) ^ (T1 & 0xffffffff) ^ -1) &
 	 ((src1 & 0xffffffff) ^ (T0 & 0xffffffff))) & (1 << 31))
 	env->psr |= PSR_OVF;
 
-    env->xcc = 0;
     if (!T0)
 	env->xcc |= PSR_ZERO;
     if ((int64_t) T0 < 0)
 	env->xcc |= PSR_NEG;
-    if (T0 < src1)
-	env->xcc |= PSR_CARRY;
     if (((src1 ^ T1 ^ -1) & (src1 ^ T0)) & (1ULL << 63))
 	env->xcc |= PSR_OVF;
 #else
@@ -444,8 +466,6 @@ void OPPROTO op_addx_T1_T0_cc(void)
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if (T0 < src1)
-	env->psr |= PSR_CARRY;
     if (((src1 ^ T1 ^ -1) & (src1 ^ T0)) & (1 << 31))
 	env->psr |= PSR_OVF;
 #endif
@@ -505,28 +525,50 @@ void OPPROTO op_subx_T1_T0(void)
 void OPPROTO op_subx_T1_T0_cc(void)
 {
     target_ulong src1;
-
     src1 = T0;
-    T0 -= T1 + FLAG_SET(PSR_CARRY);
-    env->psr = 0;
+    if (FLAG_SET(PSR_CARRY))
+    {
+      T0 -= T1 + 1;
+      env->psr = 0;
+#ifdef TARGET_SPARC64
+      if ((src1 & 0xffffffff) <= (T1 & 0xffffffff))
+        env->psr |= PSR_CARRY;
+      env->xcc = 0;
+      if (src1 <= T1)
+        env->xcc |= PSR_CARRY;
+#else
+      if (src1 <= T1)
+        env->psr |= PSR_CARRY;
+#endif
+    }
+    else
+    {
+      T0 -= T1;
+      env->psr = 0;
+#ifdef TARGET_SPARC64
+      if ((src1 & 0xffffffff) < (T1 & 0xffffffff))
+        env->psr |= PSR_CARRY;
+      env->xcc = 0;
+      if (src1 < T1)
+        env->xcc |= PSR_CARRY;
+#else
+      if (src1 < T1)
+        env->psr |= PSR_CARRY;
+#endif
+    }
 #ifdef TARGET_SPARC64
     if (!(T0 & 0xffffffff))
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if ((src1 & 0xffffffff) < (T1 & 0xffffffff))
-	env->psr |= PSR_CARRY;
     if ((((src1 & 0xffffffff) ^ (T1 & 0xffffffff)) &
 	 ((src1 & 0xffffffff) ^ (T0 & 0xffffffff))) & (1 << 31))
 	env->psr |= PSR_OVF;
 
-    env->xcc = 0;
     if (!T0)
 	env->xcc |= PSR_ZERO;
     if ((int64_t) T0 < 0)
 	env->xcc |= PSR_NEG;
-    if (src1 < T1)
-	env->xcc |= PSR_CARRY;
     if (((src1 ^ T1) & (src1 ^ T0)) & (1ULL << 63))
 	env->xcc |= PSR_OVF;
 #else
@@ -534,8 +576,6 @@ void OPPROTO op_subx_T1_T0_cc(void)
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if (src1 < T1)
-	env->psr |= PSR_CARRY;
     if (((src1 ^ T1) & (src1 ^ T0)) & (1 << 31))
 	env->psr |= PSR_OVF;
 #endif

[-- Attachment #3: qemu-sparc-carry-xcc-ops-2.patch --]
[-- Type: text/x-diff, Size: 2945 bytes --]

? qemu-sparc-carry-xcc-ops-2.patch
? qemu-sparc-carry-xcc-ops.patch
Index: op.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op.c,v
retrieving revision 1.18
diff -u -p -r1.18 op.c
--- op.c	30 Oct 2005 17:28:50 -0000	1.18
+++ op.c	10 Apr 2006 18:57:46 -0000
@@ -415,16 +415,17 @@ void OPPROTO op_addx_T1_T0(void)
 void OPPROTO op_addx_T1_T0_cc(void)
 {
     target_ulong src1;
-
+    target_ulong has_carry = FLAG_SET(PSR_CARRY);
     src1 = T0;
-    T0 += T1 + FLAG_SET(PSR_CARRY);
+    T0 += T1 + has_carry;
     env->psr = 0;
 #ifdef TARGET_SPARC64
     if (!(T0 & 0xffffffff))
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if ((T0 & 0xffffffff) < (src1 & 0xffffffff))
+    if (((T0 & 0xffffffff) < (src1 & 0xffffffff)) ||
+        (has_carry && ((T0 & 0xffffffff) <= (src1 & 0xffffffff))))
 	env->psr |= PSR_CARRY;
     if ((((src1 & 0xffffffff) ^ (T1 & 0xffffffff) ^ -1) &
 	 ((src1 & 0xffffffff) ^ (T0 & 0xffffffff))) & (1 << 31))
@@ -435,7 +436,7 @@ void OPPROTO op_addx_T1_T0_cc(void)
 	env->xcc |= PSR_ZERO;
     if ((int64_t) T0 < 0)
 	env->xcc |= PSR_NEG;
-    if (T0 < src1)
+    if (T0 < src1 || (has_carry && T0 <= src1))
 	env->xcc |= PSR_CARRY;
     if (((src1 ^ T1 ^ -1) & (src1 ^ T0)) & (1ULL << 63))
 	env->xcc |= PSR_OVF;
@@ -444,7 +445,7 @@ void OPPROTO op_addx_T1_T0_cc(void)
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if (T0 < src1)
+    if (T0 < src1 || (has_carry && T0 <= src1))
 	env->psr |= PSR_CARRY;
     if (((src1 ^ T1 ^ -1) & (src1 ^ T0)) & (1 << 31))
 	env->psr |= PSR_OVF;
@@ -505,16 +506,17 @@ void OPPROTO op_subx_T1_T0(void)
 void OPPROTO op_subx_T1_T0_cc(void)
 {
     target_ulong src1;
-
+    target_ulong has_carry = FLAG_SET(PSR_CARRY);
     src1 = T0;
-    T0 -= T1 + FLAG_SET(PSR_CARRY);
+    T0 -= T1 + has_carry;
     env->psr = 0;
 #ifdef TARGET_SPARC64
     if (!(T0 & 0xffffffff))
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if ((src1 & 0xffffffff) < (T1 & 0xffffffff))
+    if (((src1 & 0xffffffff) < (T1 & 0xffffffff)) ||
+        (has_carry && ((src1 & 0xffffffff) <= (T1 & 0xffffffff))))
 	env->psr |= PSR_CARRY;
     if ((((src1 & 0xffffffff) ^ (T1 & 0xffffffff)) &
 	 ((src1 & 0xffffffff) ^ (T0 & 0xffffffff))) & (1 << 31))
@@ -525,7 +527,7 @@ void OPPROTO op_subx_T1_T0_cc(void)
 	env->xcc |= PSR_ZERO;
     if ((int64_t) T0 < 0)
 	env->xcc |= PSR_NEG;
-    if (src1 < T1)
+    if (src1 < T1 || (has_carry && src1 <= T1))
 	env->xcc |= PSR_CARRY;
     if (((src1 ^ T1) & (src1 ^ T0)) & (1ULL << 63))
 	env->xcc |= PSR_OVF;
@@ -534,7 +536,7 @@ void OPPROTO op_subx_T1_T0_cc(void)
 	env->psr |= PSR_ZERO;
     if ((int32_t) T0 < 0)
 	env->psr |= PSR_NEG;
-    if (src1 < T1)
+    if (src1 < T1 || (has_carry && src1 <= T1))
 	env->psr |= PSR_CARRY;
     if (((src1 ^ T1) & (src1 ^ T0)) & (1 << 31))
 	env->psr |= PSR_OVF;

RE: [Qemu-devel] [PATCH] SPARC target : Fix carry flag update in addxccand subxc

[Blue Swirl]

>I send a patch that should fix a bug in the update of carry flag for addxcc
>and subxcc instructions when the carry flag is set before the evaluation of
>the instruction.
>(the fix is identical to what is done in the similar instruction
>op_adcl_T0_T1_cc for arm target)

Looks good, but what about V flag?

Instead of adding a new condition, perhaps you could store the intermediate 
results of addition, like T0 + FLAG_SET(PSR_CARRY) and use that later in the 
comparison?

BTW: ARM env->NZF idea could be used to Sparc as well.

_________________________________________________________________
FREE pop-up blocking with the new MSN Toolbar - get it now! 
http://toolbar.msn.click-url.com/go/onm00200415ave/direct/01/