[PATCH,SH] Add SH2A new instructions

Kaz Kojima kkojima@rr.iij4u.or.jp
Sun Sep 23 15:11:00 GMT 2007


"Naveen H.S." <naveen.hs@kpitcummins.com> wrote:
> Renesas has added about 36 new instructions in SH2A target. Almost 29
> of these instructions were not implemented in GCC. We had taken the 
> task of identifying these unimplemented instructions and implementing
> these in GCC in phases. 
> Following instructions are implemented in this patch,

This patch isn't appropriate for stage3 and should be re-posted
for 4.4 stage1 after 4.3 is branched.  There are some comments,
though.

First of all, the patch can't be applied to the trunk cleanly.
It seems that you missed the patch in the thread:

  http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01883.html

There are several comments about coding standard.  Please
check again our coding standard:

  http://gcc.gnu.org/contribute.html#standards

because I can't specify all issues.

It seems that your patch could be divided into 4-5 parts at
least.  It'll help not only reviewing but also investigating
which part causes problems.

> *** /gcc/config/sh/sh2a.md	1970-01-01 05:00:00.000000000 +0500
> --- /gcc/config/sh/sh2a.md	2007-08-28 17:50:02.000000000 +0500
[snip]
> + ;; GNU CC is free software; you can redistribute it and/or modify
> + ;; it under the terms of the GNU General Public License as published by
> + ;; the Free Software Foundation; either version 2, or (at your option)
> + ;; any later version.

We've moved to GPLv3.  See another current sh/*.md files.

> +   if (GET_CODE (XEXP (operands[0],0)) == REG)
> +     {
> +       operands[0] = XEXP (operands[0],0);
> +       output_asm_insn (\"bclr.b  %2,@(0, %0)\", operands);
> +       return \"\";
> +     }
> + 
> +   else if ((GET_CODE (XEXP (operands[0],0)) == PLUS)

An extra empty line before else if.  A space is needed after
'operands[0],'.
Also could you use \"bclr.b	%2,@(0,%0)\" here?  We usually
use "insn<TAB>operands" as the text for instructions and write
the operands part with no spaces.  This makes writing tests
which examine assembler outputs easy, for example.  
Several examples in sh2a.md.

> +       output_asm_insn (\"b%cnot5.b   %3,@(0,%1)\",operands);

Typo of b%c5not.b?

> +   else if ((GET_CODE (XEXP (operands[0], 0)) == MEM)
> +             && (GET_CODE (XEXP (XEXP (operands[0], 0), 0)) == PLUS)
> +             && (GET_CODE (XEXP (XEXP (XEXP (operands[0], 0), 0), 0)) == REG)
> +             && (GET_CODE (XEXP (XEXP (XEXP (operands[0], 0), 0), 1)) == CONST_INT))

Lines over about 75 columns should be wrapped.  I know that
lines in sh/* break this rule, but please don't add more.
There are several such lines in the patch.

> +    && (INTVAL(operands[1]) + INTVAL(operands[2]) >= 5
> +        && INTVAL(operands[1]) + INTVAL(operands[2]) <= 7)"
> +   "*

A space is needed after INTVAL.  Not only here.

There are no comments for new insns in sh2a.md at all.
Please add appropriate comments for new insns, though
it seems that sh2a.md has more serious problems.

> + (define_insn "bclr"
> +   [(set (zero_extract:SI (match_operand:QI 0 "memory_operand" "")
> +                          (match_operand 1 "const_int_operand" "")
> +                          (match_operand 2 "const_int_operand" ""))
> +         (const_int 0))]
> +   "TARGET_SH2A && (GET_CODE (operands[0]) == MEM)

Several insns in sh2a.md have the pattern without null constraints.
This might confuse reload.  I've got an ICE

  internal compiler error: in reload_combine_note_use, at postreload.c:1086

during tests for sh-unknown-elf with -m2a which looks to be
caused by this.  You should define a simple insn for bxxx.b
and make expanders using these insns.  It also avoids generating
multiple instructions as text which is done in your sh2a.md.
For example, a psuedo code like

(define_insn "bst"
  [(set (zero_extract:SI
	 (mem:QI (match_operand:SI 0 "reg_operand" "r"))
                         (match_operand:SI 1 "const_int_operand" "M")
                         (match_operand:SI 2 "const_int_operand" "i"))
        (reg:SI T_REG))]
  "TARGET_SH2A
   && (GET_CODE (operands[1]) == CONST_INT
       && INTVAL (operands[1]) == 1)
   && (GET_CODE (operands[2]) == CONST_INT
       && INTVAL (operands[2]) >= 2 && INTVAL (operands[2]) <= 7)
   && (INTVAL (operands[1]) + INTVAL (operands[2]) >= 3
       && INTVAL (operands[1]) + INTVAL (operands[2]) <= 7)"
  "*
{
  output_asm_insn (\"bst.b	%2,@(0,%0)\", operands);
  return \"\";
}")

(define_expand "bst_mem"
  [(set (zero_extract:SI (match_operand:QI 0 "memory_operand" "")
                         (match_operand:SI 1 "const_int_operand" "")
                         (match_operand:SI 2 "const_int_operand" ""))
        (match_operand:SI 3 "arith_reg_operand" "r"))]
  "TARGET_SH2A
  ...
{
  if (GET_CODE (XEXP (operands[0],0)) == REG)
    {
      rtx one = CONST1_RTX (SImode);
      emit_insn (gen_bld (operands[3], one, CONST0_RTX (SImode)));
      operands[0] = XEXP (operands[0], 0);
      emit_insn (gen_bst (operands[0], one, operands[2]));
    }
  ...

will work and may generate a better code.

> *** /gcc/config/sh/sh.c	2007-08-25 00:00:59.000000000 +0500
> --- /gcc/config/sh/sh.c	2007-08-28 15:43:02.000000000 +0500
[snip]
> *************** print_operand_address (FILE *stream, rtx
> *** 649,660 ****
> --- 668,681 ----
>      'R'  print the LSW of a dp value - changes if in little endian
>      'S'  print the MSW of a dp value - changes if in little endian
>      'T'  print the next word of a dp value - same as 'R' in big endian mode.
> +    'c'  output a bit operator.  
>      'M'  SHMEDIA: print an `x' if `m' will print `base,index'.
>           otherwise: print .b / .w / .l / .s / .d suffix if operand is a MEM.
>      'N'  print 'r63' if the operand is (const_int 0).
>      'd'  print a V2SF reg as dN instead of fpN.
>      'm'  print a pair `base,offset' or `base,index', for LD and ST.
>      'U'  Likewise for {LD,ST}{HI,LO}.
> +    't'  Print the operand address in x to the stream.
[snip]
> +     case 't':
> +       print_operand_address (stream, x);
> +       break;
> + 
> +     case 'V':
> +       regno = exact_log2 ((INTVAL (x)) & 0xff);
> +       gcc_assert (regno >= 0);
> +       fprintf (stream, "#%d", regno);
> +       break;
> + 
> +     case 'W':
> +       regno = exact_log2 ((~INTVAL (x)) & 0xff);
> +       gcc_assert (regno >= 0);
> +       fprintf (stream, "#%d", regno);
> +       break;
> + 
> +     case 'Y':

It looks there are no explanations for new 'V', 'W' and 'Y'
modifiers.

> *************** broken_move (rtx insn)
> *** 3710,3716 ****
>   		&& FP_REGISTER_P (REGNO (SET_DEST (pat))))
>   	  && ! (TARGET_SH2A
>   		&& GET_MODE (SET_DEST (pat)) == SImode
> ! 		&& satisfies_constraint_I20 (SET_SRC (pat)))
>   	  && ! satisfies_constraint_I08 (SET_SRC (pat)))
>   	return 1;
>       }
> --- 3798,3805 ----
>   		&& FP_REGISTER_P (REGNO (SET_DEST (pat))))
>   	  && ! (TARGET_SH2A
>   		&& GET_MODE (SET_DEST (pat)) == SImode
> !                 && (CONST_OK_FOR_I20 (INTVAL (SET_SRC (pat)))
> !                 || CONST_OK_FOR_I28 (INTVAL (SET_SRC (pat)))))	
>   	  && ! satisfies_constraint_I08 (SET_SRC (pat)))
>   	return 1;
>       }

Use satisfies_constraint_I2[08] instead of CONST_OK_FOR_I2[08]
that are added in sh.h.  We shouldn't add new CONST_OK_FOR_*
macros anyway.

> !   	/* If the ISR has RESBANK attribute assigned, don't push any of the
> !    	   the following registers - R0-R14, MACH, MACL and GBR */

All English sentences should be ended with a period and two
spaces are required after it.  Several examples.

> *************** push_regs (HARD_REG_SET *mask, int inter
> *** 5748,5754 ****
>         if (i != PR_REG
>   	  && (i != FPSCR_REG || ! skip_fpscr)
>   	  && TEST_HARD_REG_BIT (*mask, i))
> ! 	push (i);
>       }
>   
>     /* Push banked registers last to improve delay slot opportunities.  */
> --- 5837,5853 ----
>         if (i != PR_REG
>   	  && (i != FPSCR_REG || ! skip_fpscr)
>   	  && TEST_HARD_REG_BIT (*mask, i))
> !            {
> !   	/* If the ISR has RESBANK attribute assigned, don't push any of the
> !    	   the following registers - R0-R14, MACH, MACL and GBR */
> ! 	  if(! (sh_cfun_resbank_handler_p()
> !                    && (((i >= FIRST_GENERAL_REG) 
> ! 	           && (i < LAST_GENERAL_REG))
> ! 	           || (i == MACH_REG)
> ! 	           || (i == MACL_REG)
> !                    || (i == GBR_REG))))
> ! 	  push (i);
> !   	}
>       }
>   
>     /* Push banked registers last to improve delay slot opportunities.  */

Wrong indentation.  A space is needed between 'if' and '('
and between a function symbol and '('.  Also you don't need
the extra parenthesis around the comparisons.
Several examples.

> *************** sh_expand_epilogue (bool sibcall_p)
> *** 6714,6720 ****
>   	      && hard_reg_set_intersect_p (live_regs_mask,
>   					  reg_class_contents[DF_REGS]))
>   	    fpscr_deferred = 1;
> ! 	  else if (j != PR_REG && TEST_HARD_REG_BIT (live_regs_mask, j))
>   	    pop (j);
>   
>   	  if (j == FIRST_FP_REG && fpscr_deferred)
> --- 6815,6829 ----
>   	      && hard_reg_set_intersect_p (live_regs_mask,
>   					  reg_class_contents[DF_REGS]))
>   	    fpscr_deferred = 1;
> ! 	  /* For an ISR with RESBANK attribute assigned, don't pop following 
> !              registers, R0-R14, MACH, MACL and GBR. */
> ! 	  else if (j != PR_REG && TEST_HARD_REG_BIT (live_regs_mask, j) 
> ! 		   && ! (sh_cfun_resbank_handler_p()  
> ! 			 && (((j >= FIRST_GENERAL_REG)
> !                                && (j < LAST_GENERAL_REG)) 
> !                              || (j == MACH_REG)
> !                              || (j == MACL_REG)
> !                              || (j == GBR_REG))))
>   	    pop (j);
>   
>   	  if (j == FIRST_FP_REG && fpscr_deferred)

It'd be better to make

  (sh_cfun_resbank_handler_p ()
   && (i >= FIRST_GENERAL_REG && i < LAST_GENERAL_REG)
       || i == MACH_REG
       || i == MACL_REG
       || i == GBR_REG))

(I removed extra parenthesis here) into a separate small
function.  You could use it in push_regs and sh_expand_epilogue.

> + sh2a_get_function_vector_number (rtx x)
[snip]
> +     {
> +       tree t = SYMBOL_REF_DECL (x);
> + 
> +       if (TREE_CODE (t) != FUNCTION_DECL)
> +         return 0;
> + 
> +       tree list = SH_ATTRIBUTES (t);

Don't mix declarations and statements.

+ sh2a_function_vector_p (tree func)
+ {
+   if (TREE_CODE (func) != FUNCTION_DECL)
+     return 0;
+ 
+   tree list = SH_ATTRIBUTES (func);

Same here.

> + }
> + 
> + int
> + sh_cfun_resbank_handler_p (void)

Please add a comment for this new function.

> *** /gcc/config/sh/sh.h	2007-08-02 15:49:31.000000000 +0500
> --- /gcc/config/sh/sh.h	2007-08-28 16:12:14.000000000 +0500
[snip]
> + #define CONST_OK_FOR_I20(VALUE) (((HOST_WIDE_INT)(VALUE)) >= -524288 \
> +                                  && ((HOST_WIDE_INT)(VALUE)) <= 524287 \
> +                                  && TARGET_SH2A)
> + #define CONST_OK_FOR_I28(VALUE) ((((HOST_WIDE_INT)(VALUE)) >= -134217728)\
> +                                   && (((HOST_WIDE_INT)(VALUE)) <= 134217727)\
> +                                  && (((HOST_WIDE_INT)(VALUE) & (HOST_WIDE_INT) 0x000000FF) == 0)\
> +                                  && TARGET_SH2A)

As mentioned above, don't add new CONST_OK_FOR_* macros.

> *** /gcc/config/sh/sh.md	2007-08-02 15:49:31.000000000 +0500
> --- /gcc/config/sh/sh.md	2007-08-28 17:28:25.000000000 +0500
[snip]
> *** 11610,11617 ****
>   
>     FAIL;
>   })
> - 
> - 
>   ;; -------------------------------------------------------------------------
>   ;; Peepholes
>   ;; -------------------------------------------------------------------------
Don't do this.

> *** /gcc/config/sh/sh-protos.h	2007-08-23 20:49:56.000000000 +0500
> --- /gcc/config/sh/sh-protos.h	2007-08-28 15:15:51.000000000 +0500
[snip]
> *************** struct secondary_reload_info;
> *** 173,178 ****
> --- 174,181 ----
>   extern enum reg_class sh_secondary_reload (bool, rtx, enum reg_class,
>   					   enum machine_mode,
>   					   struct secondary_reload_info *);
> + int sh2a_get_function_vector_number (rtx);
> + int sh2a_is_function_vector_call (rtx);

Use extern for consistency.

> *** /gcc/doc/extend.texi	2007-07-25 17:28:31.000000000 +0500
> --- /gcc/doc/extend.texi	2007-08-29 09:28:38.259039736 +0500
> *************** function through the function vector wil
> *** 2061,2066 ****
> --- 2061,2084 ----
>   the function vector has a limited size (maximum 128 entries on the H8/300
>   and 64 entries on the H8/300H and H8S) and shares space with the interrupt vector.
>   
> + In SH2A target, this attribute declares a function to be called using the
> + TBR relative addressing mode. The argument to this attribute is the entry
> + number of the same function in a vector table containing all the "TBR
> + relative" addressable functions. For the successful jump, register TBR
> + should contain the start address of this "TBR relative" vector table.
> + In the startup routine of the user application, user needs to care of this
> + TBR register initialization. The "TBR relative" vector table can have at
> + max 256 function entries. The jumps to these functions will be generated
> + using a SH2A specific, non delayed branch instruction "JSR/N @@(disp8,TBR).
> + 
> + Please refer the above example of M16C target, to see the use of this
> + attribute while declaring a function,
> + 
> + In an application, for a function being called once, this attribute will
> + save at least 8 bytes of code; and if other successive calls are being
> + made to the same function, it will save 2 bytes of code per each of these
> + calls.

Two spaces after the end of sentences.  It looks that double
quotes are not needed.
As the item for function_vector starts with:

@cindex calling functions through the function vector on H8/300, M16C, and M32C
processors

SH2A should be added to this list of processors.

> --- /gcc/testsuite/gcc.dg/sh2a-resbank.c	2007-08-28 16:55:14.000000000 +0500
[snip]
> --- /gcc/testsuite/gcc.dg/sh2a-tbr-jump.c	2007-08-28 16:55:42.000000000 +0500

These tests should go to gcc/testsuite/gcc.target/sh/.

With the patch, sh4-unknown-linux-gnu doesn't bootstrap.
The first error is

/exp/ldroot/dodes/sh-gcc/./prev-gcc/xgcc -B/exp/ldroot/dodes/sh-gcc/./prev-gcc/ -B/usr/gnu/sh4-unknown-linux-gnu/bin/ -c   -g -O2 -DIN_GCC   -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros     -Wno-overlength-strings -Werror -fno-common   -DHAVE_CONFIG_H -I. -I. -I../../INTEST/trunk/gcc -I../../INTEST/trunk/gcc/. -I../../INTEST/trunk/gcc/../include -I../../INTEST/trunk/gcc/../libcpp/include  -I../../INTEST/trunk/gcc/../libdecnumber -I../../INTEST/trunk/gcc/../libdecnumber/dpd -I../libdecnumber    insn-output.c -o insn-output.o
cc1: warnings being treated as errors
../../INTEST/trunk/gcc/config/sh/sh2a.md: In function 'output_7':
../../INTEST/trunk/gcc/config/sh/sh2a.md:145: error: unused variable 'z'
../../INTEST/trunk/gcc/config/sh/sh2a.md:166: error: unused variable 'z'
../../INTEST/trunk/gcc/config/sh/sh2a.md: In function 'output_8':
../../INTEST/trunk/gcc/config/sh/sh2a.md:246: error: unused variable 'z'
../../INTEST/trunk/gcc/config/sh/sh2a.md:267: error: unused variable 'z'
../../INTEST/trunk/gcc/config/sh/sh2a.md: In function 'output_10':
../../INTEST/trunk/gcc/config/sh/sh2a.md:394: error: unused variable 'z'
make[3]: *** [insn-output.o] Error 1

Remove these unused variables.
Then the second error is

/exp/ldroot/dodes/sh-gcc/./prev-gcc/xgcc -B/exp/ldroot/dodes/sh-gcc/./prev-gcc/ -B/usr/gnu/sh4-unknown-linux-gnu/bin/ -c   -g -O2 -DIN_GCC   -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros     -Wno-overlength-strings -Werror -fno-common   -DHAVE_CONFIG_H -I. -I. -I../../INTEST/trunk/gcc -I../../INTEST/trunk/gcc/. -I../../INTEST/trunk/gcc/../include -I../../INTEST/trunk/gcc/../libcpp/include  -I../../INTEST/trunk/gcc/../libdecnumber -I../../INTEST/trunk/gcc/../libdecnumber/dpd -I../libdecnumber    insn-output.c -o insn-output.o
cc1: warnings being treated as errors
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_18':
../../INTEST/trunk/gcc/config/sh/sh2a.md:626: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_17':
../../INTEST/trunk/gcc/config/sh/sh2a.md:602: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_15':
../../INTEST/trunk/gcc/config/sh/sh2a.md:563: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_14':
../../INTEST/trunk/gcc/config/sh/sh2a.md:530: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_13':
../../INTEST/trunk/gcc/config/sh/sh2a.md:464: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_10':
../../INTEST/trunk/gcc/config/sh/sh2a.md:410: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_9':
../../INTEST/trunk/gcc/config/sh/sh2a.md:324: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_8':
../../INTEST/trunk/gcc/config/sh/sh2a.md:282: error: control reaches end of non-void function
../../INTEST/trunk/gcc/config/sh/sh.md: In function 'output_7':
../../INTEST/trunk/gcc/config/sh/sh2a.md:181: error: control reaches end of non-void function
make[3]: *** [insn-output.o] Error 1

These errors correspond to define_insn which lacks a return
statement.  Perhaps, calling gcc_unreachable at the end of
C statements would be better in some cases, though many of
these insns should be replaced with expanders in the first
place.

I've got the following regressions for C testsuite on
sh-unknown-elf with RUNTESTFLAGS=--target_board=sh-sim/-m2a:

sh-sim/-m2a: gcc.c-torture/execute/20040629-1.c compilation,  -Os  (internal compiler error)
sh-sim/-m2a: gcc.c-torture/execute/20040705-1.c compilation,  -O2  (internal compiler error)
sh-sim/-m2a: gcc.c-torture/execute/20040705-1.c compilation,  -Os  (internal compiler error)
sh-sim/-m2a: gcc.c-torture/execute/20040705-2.c compilation,  -Os  (internal compiler error)
sh-sim/-m2a: gcc.dg/sh2a-tbr-jump.c scan-assembler-times jsr    @r[0-9] 2

The first 4 errors are ICEs with

  internal compiler error: in reload_combine_note_use, at postreload.c:1086

which are mentioned above.

Regards,
	kaz



More information about the Gcc-patches mailing list