Re: [Qemu-devel] [PATCH][SPARC] Fix the shift instructions for theSPARC target

[Aurelien Jarno]

On Sun, Apr 01, 2007 at 11:32:06AM +0200, Blue Swirl wrote:
> >The shift instructions on the SPARC target currently take into account
> >the whole register as the shift count. According to the SPARC v8 and v9
> >manuals, only the lower 5 bits should be taken into account for 32-bit
> >instructions (SLL, SRL, SRA), and only the lower 6 bits for 64-bit
> >instructions (SLLX, SRLX, SRAX).
> >
> >The patch below fixes that. Note that SLL and SLLX are now different, as
> >they don't take into account the same number of bits. Please apply.
> 
> Can you check what happens in real hardware, especially in the case when 
> the shift amount is in a register, not immediate, and the value is either 
> >32 or ==32?

I have just made the test on real hardware. Specifying a value of 32 leave
the register unchanged. Specifying a value > 32 only takes the lower 5
bits of the value. So exactly as specified in the manual.

> The 64-bit mask should be 0x3f, not 0x2f.

Oops you are right, please find an updated patch below.


Index: target-sparc/op.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op.c,v
retrieving revision 1.26
diff -u -d -p -r1.26 op.c
--- target-sparc/op.c   23 Mar 2007 20:01:20 -0000      1.26
+++ target-sparc/op.c   25 Mar 2007 13:50:45 -0000
@@ -965,38 +965,43 @@ void OPPROTO op_logic_T0_cc(void)
 
 void OPPROTO op_sll(void)
 {
-    T0 <<= T1;
+    T0 <<= (T1 & 0x1f);
 }
 
 #ifdef TARGET_SPARC64
+void OPPROTO op_sllx(void)
+{
+    T0 <<= (T1 & 0x3f);
+}
+
 void OPPROTO op_srl(void)
 {
-    T0 = (T0 & 0xffffffff) >> T1;
+    T0 = (T0 & 0xffffffff) >> (T1 & 0x1f);
 }
 
 void OPPROTO op_srlx(void)
 {
-    T0 >>= T1;
+    T0 >>= (T1 & 0x3f);
 }
 
 void OPPROTO op_sra(void)
 {
-    T0 = ((int32_t) (T0 & 0xffffffff)) >> T1;
+    T0 = ((int32_t) (T0 & 0xffffffff)) >> (T1 & 0x1f);
 }
 
 void OPPROTO op_srax(void)
 {
-    T0 = ((int64_t) T0) >> T1;
+    T0 = ((int64_t) T0) >> (T1 & 0x3f);
 }
 #else
 void OPPROTO op_srl(void)
 {
-    T0 >>= T1;
+    T0 >>= (T1 & 0x1f);
 }
 
 void OPPROTO op_sra(void)
 {
-    T0 = ((int32_t) T0) >> T1;
+    T0 = ((int32_t) T0) >> (T1 & 0x1f);
 }
 #endif
 
Index: target-sparc/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/translate.c,v
retrieving revision 1.38
diff -u -d -p -r1.38 translate.c
--- target-sparc/translate.c    25 Mar 2007 07:55:52 -0000      1.38
+++ target-sparc/translate.c    25 Mar 2007 13:50:45 -0000
@@ -1693,7 +1693,7 @@ static void disas_sparc_insn(DisasContex
                }
 #endif
 #ifdef TARGET_SPARC64
-           } else if (xop == 0x25) { /* sll, V9 sllx ( == sll) */
+           } else if (xop == 0x25) { /* sll, V9 sllx */
                 rs1 = GET_FIELD(insn, 13, 17);
                gen_movl_reg_T0(rs1);
                if (IS_IMM) {   /* immediate */
@@ -1703,7 +1703,10 @@ static void disas_sparc_insn(DisasContex
                     rs2 = GET_FIELD(insn, 27, 31);
                     gen_movl_reg_T1(rs2);
                 }
-               gen_op_sll();
+               if (insn & (1 << 12))
+                   gen_op_sllx();
+               else
+                   gen_op_sll();
                gen_movl_T0_reg(rd);
            } else if (xop == 0x26) { /* srl, V9 srlx */
                 rs1 = GET_FIELD(insn, 13, 17);
 


-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net