Re: [PATCH] : SH Assembler generates incorrect opcode for PCMP instructions

[Nick Clifton <nickc@cambridge.redhat.com>]

Hi Arati,

> The SH assembler generates incorrect opcodes for the parallel PCMP 
> instruction.  Basically, its last nibble is a copy of the previous
> instruction's second nibble when working in Big Endian format.
> 
> For example,
> test.s
> 	movs.w @-R5, A0
>       PCMP X0,Y0
> 
> generates opcodes
> 	f5 70
> 	f8 00 84 05
> 
>  instead of
> 	f8 00 84 00
> 
> The following patch corrects this. I have also verified that it does
> not cause any side effect on other DSP instructions.

>  	  if (field_b)
>  	    as_bad (_("multiple parallel processing specifications"));
>  	  field_b = ((opcode->nibbles[2] << 12) + (opcode->nibbles[3] <<
> 8)
> -		     + (reg_x << 6) + (reg_y << 4) + reg_n);
> +		     + (reg_x << 6) + (reg_y << 4) );
> +
> +	  if (strcmp (opcode->name, "pcmp") != 0)
> +	  	 field_b += reg_n;
>  	  break;
>  	case PDC:
>  	  if (cond)

Your patch does work, but I do not like the way it makes a special
case out of the opcode's name.  In fact I think that it may not be
just the PCMP instruction that has this problem, since a perusal of
the sh-opc.h file indicates that the PABS and PRND instructions may
also have similar difficulties.

Please could you try the patch below, which I believe provides a more
generic fix for the problem, and which also adds some tests to the SH
gas testsuite to make sure that the problem does not arise again in
the future.

Cheers
        Nick

gas/ChangeLog
2002-01-31  Nick Clifton  <nickc@cambridge.redhat.com>

	* config/tc-sh.c (opcode_has_arg): New function.
        (assemble_ppi): For case PPI3 only insert those registers
        which are specified in the opcode's arg array.

gas/testsuite/ChangeLog
2002-01-31  Nick Clifton  <nickc@cambridge.redhat.com>

	* gas/sh/dsp.s: Add tests of a few DSP instructions.
        * gas/sh/dsp.d: Add expected disassembly.

Index: gas/config/tc-sh.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-sh.c,v
retrieving revision 1.53
diff -c -3 -p -w -r1.53 tc-sh.c
*** tc-sh.c	2002/01/30 18:25:30	1.53
--- tc-sh.c	2002/01/31 21:38:50
***************
*** 29,34 ****
--- 29,35 ----
  #include "opcodes/sh-opc.h"
  #include "safe-ctype.h"
  #include "struc-symbol.h"
+ #include "libiberty.h"
  
  #ifdef OBJ_ELF
  #include "elf/sh.h"
*************** static void build_relax PARAMS ((sh_opco
*** 72,77 ****
--- 73,79 ----
  static char *insert_loop_bounds PARAMS ((char *, sh_operand_info *));
  static unsigned int build_Mytes
    PARAMS ((sh_opcode_info *, sh_operand_info *));
+ static boolean opcode_has_arg PARAMS ((sh_opcode_info *, sh_arg_type));
  
  #ifdef OBJ_ELF
  static void sh_elf_cons PARAMS ((int));
*************** find_cooked_opcode (str_p)
*** 1679,1684 ****
--- 1681,1702 ----
    return (sh_opcode_info *) hash_find (opcode_hash_control, name);
  }
  
+ /* Returns true if OPCODE has an arg of type ARG.  */
+ 
+ static boolean
+ opcode_has_arg (opcode, arg)
+      sh_opcode_info * opcode;
+      sh_arg_type arg;
+ {
+   unsigned int i;
+ 
+   for (i = 0; i < ARRAY_SIZE (opcode->arg); i++)
+     if (opcode->arg[i] == arg)
+       return true;
+ 
+   return false;
+ }
+ 
  /* Assemble a parallel processing insn.  */
  #define DDT_BASE 0xf000 /* Base value for double data transfer insns */
  
*************** assemble_ppi (op_end, opcode)
*** 1791,1798 ****
  	case PPI3:
  	  if (field_b)
  	    as_bad (_("multiple parallel processing specifications"));
! 	  field_b = ((opcode->nibbles[2] << 12) + (opcode->nibbles[3] << 8)
! 		     + (reg_x << 6) + (reg_y << 4) + reg_n);
  	  break;
  	case PDC:
  	  if (cond)
--- 1809,1824 ----
  	case PPI3:
  	  if (field_b)
  	    as_bad (_("multiple parallel processing specifications"));
!  	  field_b = ((opcode->nibbles[2] << 12) + (opcode->nibbles[3] << 8));
! 
! 	  if (opcode_has_arg (opcode, DSP_REG_X))
! 	    field_b |= (reg_x << 6);
! 
! 	  if (opcode_has_arg (opcode, DSP_REG_Y))
! 	    field_b |= (reg_y << 4);
!       
! 	  if (opcode_has_arg (opcode, DSP_REG_N))
! 	    field_b |= reg_n;
  	  break;
  	case PDC:
  	  if (cond)

Index: gas/testsuite/gas/sh/dsp.s
===================================================================
RCS file: /cvs/src/src/gas/testsuite/gas/sh/dsp.s,v
retrieving revision 1.1
diff -c -3 -p -w -r1.1 dsp.s
*** dsp.s	2001/10/09 12:25:52	1.1
--- dsp.s	2002/01/31 21:38:50
***************
*** 1,4 ****
! #	Test file for ARM/GAS -- basic instructions
  
  	.text
  	.align
--- 1,4 ----
! #	Test file for SH DSP assembler instructions.
  
  	.text
  	.align
*************** dsp_tests:
*** 22,24 ****
--- 22,36 ----
  	movs.l m0, @r3+
  	movs.l m1, @r2+r8
  
+ 	# The following tests reproduce a bug discovered in the toolchain
+ 	# The symptom was that insns encoded with the PPI3 attribute would
+ 	# always have three registers encoded into their bitfields, even if
+ 	# they did not actually accept three registers.  The "third" register
+ 	# would come from whatever value was used for the previous instruction.
+ 	movs.w @-R5, A0
+ 	pcmp   X0, Y0
+ 	psubc  X1, Y1, A1
+ 	pabs   X0, A0
+ 	pabs   Y0, A1
+ 	prnd   X1, M0
+ 	prnd   Y1, A1G

Index: gas/testsuite/gas/sh/dsp.d
===================================================================
RCS file: /cvs/src/src/gas/testsuite/gas/sh/dsp.d,v
retrieving revision 1.2
diff -c -3 -p -w -r1.2 dsp.d
*** dsp.d	2002/01/15 12:27:53	1.2
--- dsp.d	2002/01/31 21:38:50
*************** Disassembly of section .text:
*** 22,24 ****
--- 22,31 ----
  0+01a <[^>]*> f4 b7 [ 	]*movs.l	y1,@r4
  0+01c <[^>]*> f7 cb [ 	]*movs.l	m0,@r3\+
  0+01e <[^>]*> f6 ef [ 	]*movs.l	m1,@r2\+r8
+ 0+020 <[^>]*> f5 70 [ 	]*movs.w	@-r5,a0
+ 0+022 <[^>]*> f8 00 84 00 [ 	]*pcmp	x0,y0
+ 0+026 <[^>]*> f8 00 a0 55 [ 	]*psubc	x1,y1,a1
+ 0+02a <[^>]*> f8 00 88 07 [ 	]*pabs	x0,a0
+ 0+02e <[^>]*> f8 00 a8 05 [ 	]*pabs	y0,a1
+ 0+032 <[^>]*> f8 00 98 4c [ 	]*prnd	x1,m0
+ 0+036 <[^>]*> f8 00 b8 1d [ 	]*prnd	y1,a1g