This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH,SH] Add SH2A new instructions 1/6


"Naveen H.S." <naveen.hs@kpitcummins.com> wrote:
>>> check once more our coding standard.
> Thank you for the valuable comments. We have taken care of the
> comments in the modified patch. 
>
> Please find attached the patch "sh2a1.patch". This patch implements new
> instructions RTS/N, JSR/N, JSR/N @@ and RESBANK for SH2A target.

Patch looks Ok logically, but there are still many issues about
coding standard.  Check once more our coding standard, carefully.

> No new regressions.

Please describe how the patch was tested more explicitly next time.
Did you run C and C++ testsuite with all -m2a* -ml/mb combinations?
You should refer about make dvi, make info and make pdf because
you touch a doc file.

Here is a list of the coding standard issues I've found, though
there may be more.  Please find and fix them yourself and re-post
the revised patch when the trunk returns to stage1.

> --- gcc-4.3-20070921/gcc/config/sh/sh.c	2007-09-19 20:33:07.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/config/sh/sh.c	2007-10-24 11:48:18.000000000 +0530
> @@ -69,6 +69,14 @@ int code_for_indirect_jump_scratch = COD
>  #define GEN_ADD3 (*(TARGET_SHMEDIA64 ? gen_adddi3 : gen_addsi3))
>  #define GEN_SUB3 (*(TARGET_SHMEDIA64 ? gen_subdi3 : gen_subsi3))
>  
> +/* Used to simplify the logic below.  Find the attributes wherever
> +   they may be.  */
> +#define SH_ATTRIBUTES(decl) \
> +  (TYPE_P (decl)) ? TYPE_ATTRIBUTES (decl) \
> +                : DECL_ATTRIBUTES (decl) \
> +                  ? (DECL_ATTRIBUTES (decl)) \
> +                  : TYPE_ATTRIBUTES (TREE_TYPE (decl))
   ^^^^^^^^^^^^^^^^
Use tabs.

> +static tree sh_handle_resbank_handler_attribute (tree *, tree, 
                                                                 ^
Extra trailing space.

> +static int sh2a_function_vector_p (tree);
> +
>  
>  

Don't add an extra empty line here.

> @@ -1016,6 +1042,25 @@ print_operand (FILE *stream, rtx x, int 
>      }
>  }
>  
> +
> +/* Encode symbol attributes of a SYMBOL_REF into its
> +   SYMBOL_REF_FLAGS.  */
> +static void
> +sh_encode_section_info (tree decl, rtx rtl, int first)
> +{
> +  int extra_flags = 0;
> +
> +  default_encode_section_info (decl, rtl, first);
> +
> +  if (TREE_CODE (decl) == FUNCTION_DECL
> +      && sh2a_function_vector_p (decl)
> +      && TARGET_SH2A)
> +    extra_flags = SYMBOL_FLAG_FUNCVEC_FUNCTION;
> +
> +  if (extra_flags)
> +    SYMBOL_REF_FLAGS (XEXP (rtl, 0)) |= extra_flags;
> +}

It looks that you don't need extra_flags variable:

sh_encode_section_info (tree decl, rtx rtl, int first)
{
  default_encode_section_info (decl, rtl, first);

  if (TREE_CODE (decl) == FUNCTION_DECL
      && sh2a_function_vector_p (decl)
      && TARGET_SH2A)
    SYMBOL_REF_FLAGS (XEXP (rtl, 0)) |= SYMBOL_FLAG_FUNCVEC_FUNCTION;
}

> @@ -5750,7 +5795,16 @@ push_regs (HARD_REG_SET *mask, int inter
[snip]
>        if (i != PR_REG
>  	  && (i != FPSCR_REG || ! skip_fpscr)
>  	  && TEST_HARD_REG_BIT (*mask, i))
> -	push (i);
> +           {
> +  	/* If the ISR has RESBANK attribute assigned, don't push any of the
> +   	   the following registers - R0-R14, MACH, MACL and GBR.  */

The last 3 lines have still wrong indentations.  Extra "the".
Should be

	{
	  /* If the ISR has RESBANK attribute assigned, don't push any of
	     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)))
    ^^^^^^^^^^^^^^^^
Use tabs.

> @@ -5759,7 +5813,8 @@ push_regs (HARD_REG_SET *mask, int inter
>        if (TEST_HARD_REG_BIT (*mask, i))
>  	push (i);
>  
> -  if (TEST_HARD_REG_BIT (*mask, PR_REG))
> +  /* Don't push PR register for an ISR with RESBANK attribute assigned.  */
> +  if (TEST_HARD_REG_BIT (*mask, PR_REG) && !(sh_cfun_resbank_handler_p ()))

Remove () around sh_cfun_resbank_handler_p ().

>  }
>  
> @@ -6688,7 +6743,10 @@ sh_expand_epilogue (bool sibcall_p)
>        int last_reg;
>  
>        save_size = 0;
> -      if (TEST_HARD_REG_BIT (live_regs_mask, PR_REG))
> +      /* For an ISR with RESBANK attribute assigned, don't pop PR
> +         register.  */
   ^^^^^^^^
Use a tab.

> +      if (TEST_HARD_REG_BIT (live_regs_mask, PR_REG)
> +          && !(sh_cfun_resbank_handler_p ()))	
   ^^^^^^^^
Same here.  Remove () around sh_cfun_resbank_handler_p ().

> @@ -6716,7 +6774,15 @@ sh_expand_epilogue (bool sibcall_p)
>  	      && 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))
> +	  /* 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)))
   ^^^^^^^^^^^^^^^^^^^^^^^^
Use tabs.  Wrong indentations at the last 3 lines.
Also many extra trailing spaces.

> @@ -7946,6 +8015,8 @@ sh_insert_attributes (tree node, tree *a
>     renesas -- use Renesas calling/layout conventions (functions and
>     structures).
>  
> +   resbank -- In case of an ISR, use a register bank to save registers 
                                                                         ^
Extra trailing space.

> +   R0-R14, MACH, MACL, GBR and PR. This is useful only on SH2A targets.
                                     ^^ 2 spaces are needed.

> +/* Handle a "resbank" attribute  */

Missing period.

> +static tree
> +sh_handle_resbank_handler_attribute (tree *node, tree name,
> +                                     tree args ATTRIBUTE_UNUSED,
> +                                     int flags ATTRIBUTE_UNUSED,
> +                                     bool *no_add_attrs)
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Use tabs.

> +      warning (OPT_Wattributes, "%qs attribute is supported only for SH2A",
> +               IDENTIFIER_POINTER (name));
   ^^^^^^^^
Same here

> +      warning (OPT_Wattributes, "%qs attribute only applies to functions",
> +               IDENTIFIER_POINTER (name));
   ^^^^^^^^
and here.

> +static tree
> +sh2a_handle_function_vector_handler_attribute (tree *node, tree name,
> +                                               tree args ATTRIBUTE_UNUSED,
> +                                               int flags ATTRIBUTE_UNUSED,
> +                                               bool *no_add_attrs)
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Use tabs.

> +      warning (OPT_Wattributes, "%qs attribute only applies to SH2A",
> +               IDENTIFIER_POINTER (name));
[snip]
> +      warning (OPT_Wattributes, "%qs attribute only applies to functions",
> +               IDENTIFIER_POINTER (name));
[snip]
> +      warning (OPT_Wattributes,
> +               "`%s' attribute argument not an integer constant",
> +               IDENTIFIER_POINTER (name));
[snip]
> +      warning (OPT_Wattributes,
> +               "`%s' attribute argument should be between 0 to 255",
> +               IDENTIFIER_POINTER (name));
   ^^^^^^^^
Another examples.

> +/* Returns 1 if current function has been assigned the attribute
> +   "function_vector"  */

Missing period.

> +sh2a_is_function_vector_call (rtx x)
[snip]
> +      if (sh2a_function_vector_p (tr))
> +        return 1;
   ^^^^^^^^
Use a tab.

> +/* Returns the function vector number, if the the attribute 
                                                              ^
Extra trailing space.

> +sh2a_get_function_vector_number (rtx x)
> +{
[snip]
> +      if (TREE_CODE (t) != FUNCTION_DECL)
> +        return 0;
   ^^^^^^^^
Same here.

> +      list = SH_ATTRIBUTES (t);
> +      while (list)
> +        {
> +          if (is_attribute_p ("function_vector", TREE_PURPOSE (list)))
> +            {
> +              num = TREE_INT_CST_LOW (TREE_VALUE (TREE_VALUE (list)));
> +              return num;
> +            }
> +
> +          list = TREE_CHAIN (list);
> +        }
> +
> +      return 0;

Same here.

> +/* Returns 1 if FUNC has been assigned the attribute 
                                                       ^
Extra trailing space.

> +   "function_vector"  */

Missing period.

> +sh2a_function_vector_p (tree func)
[snip]
> +  while (list)
> +    {
> +      if (is_attribute_p ("function_vector", TREE_PURPOSE (list)))
> +        return 1;
   ^^^^^^^^
Use a tab.

> +sh_cfun_resbank_handler_p (void)
> +{
> +  return ((lookup_attribute ("resbank",
> +                              DECL_ATTRIBUTES (current_function_decl)) 
                                                                         ^
Extra trailing space

> +           != NULL_TREE)
> +           && (lookup_attribute ("interrupt_handler",
> +                                  DECL_ATTRIBUTES (current_function_decl)) 
                                                                             ^
and here.

> +           != NULL_TREE)
> +	  && TARGET_SH2A);

Wrong indentations.  Also use tabs.

>  static const char *
> diff -upr gcc-4.3-20070921/gcc/config/sh/sh.md tars/gcc-4.3-20070921/gcc/config/sh/sh.md
> --- gcc-4.3-20070921/gcc/config/sh/sh.md	2007-09-12 09:16:53.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/config/sh/sh.md	2007-10-24 12:00:23.000000000 +0530
> @@ -7437,7 +7437,14 @@ label:
>     (use (reg:PSI FPSCR_REG))
>     (clobber (reg:SI PR_REG))]
>    "TARGET_SH1"
> -  "jsr	@%0%#"
> +  "*
> +   {
> +       if (TARGET_SH2A && (dbr_sequence_length () == 0))
> +           return \"jsr/n\\t@%0\";
> +       else
> +           return \"jsr\\t@%0%#\";
   ^^^^^^^^
Use tabs.

> +(define_insn "calli_tbr_rel"
> +  [(call (mem (match_operand:SI 0 "symbol_ref_operand" ""))
> +         (match_operand 1 "" ""))
   ^^^^^^^
Use a tab.

> +   (use (reg:PSI FPSCR_REG))
> +   (clobber (reg:SI PR_REG))]
> +  "TARGET_SH2A && sh2a_is_function_vector_call (operands[0])"
> +  "*
> +{
> +  unsigned HOST_WIDE_INT vect_num = sh2a_get_function_vector_number (operands[0]);

Long line.

> +  return \"jsr/n\\t@@(%O2,tbr)\";
> +}"
> +  [(set_attr "type" "call")
> +   (set (attr "fp_mode")
> +        (if_then_else (eq_attr "fpu_single" "yes")
> +                      (const_string "single") (const_string "double")))
   ^^^^^^^^^^^^^^^^
Use tabs.

> +       if (TARGET_SH2A && (dbr_sequence_length () == 0))
> +           return \"jsr/n\\t@%1\";
> +       else
> +           return \"jsr\\t@%1%#\";
   ^^^^^^^^
Use a tab.

> +(define_insn "call_valuei_tbr_rel"
> +  [(set (match_operand 0 "" "=rf")
> +        (call (mem:SI (match_operand:SI 1 "symbol_ref_operand" ""))
> +              (match_operand 2 "" "")))
> +;;   (use (reg:PSI TBR_REG))

Remove this comment.

> +{
> +  unsigned HOST_WIDE_INT vect_num = sh2a_get_function_vector_number (operands[1]);

Long line.

> +  return \"jsr/n\\t@@(%O3,tbr)\";
> +}"
> +  [(set_attr "type" "call")
> +   (set (attr "fp_mode")
> +        (if_then_else (eq_attr "fpu_single" "yes")
> +                      (const_string "single") (const_string "double")))
   ^^^^^^^^^^^^^^^^
Use tabs.

> +   (set_attr "needs_delay_slot" "no")
> +   (set_attr "fp_set" "unknown")])
> +
>  (define_insn "call_valuei_pcrel"
>    [(set (match_operand 0 "" "=rf")
>  	(call (mem:SI (match_operand:SI 1 "arith_reg_operand" "r"))
> @@ -7709,6 +7772,16 @@ label:
>        emit_insn (gen_symGOTPLT2reg (reg, XEXP (operands[0], 0)));
>        XEXP (operands[0], 0) = reg;
>      }
> +  if (!flag_pic && TARGET_SH2A
> +     && GET_CODE (operands[0]) == MEM
> +     && GET_CODE (XEXP (operands[0], 0)) == SYMBOL_REF)

Wrong indentations at the last 2 lines.

> +      if (sh2a_is_function_vector_call (XEXP (operands[0], 0)))
> +        {
> +          emit_call_insn (gen_calli_tbr_rel (XEXP (operands[0], 0), operands[1]));

Long line.

> @@ -7892,6 +7965,16 @@ label:
[snip]
> +          emit_call_insn (gen_call_valuei_tbr_rel (operands[0], XEXP (operands[1], 0), operands[2]));

Long line.  Use a tab.

> +          DONE;
   ^^^^^^^^
Use a tab.

> --- gcc-4.3-20070921/gcc/doc/extend.texi	2007-09-20 21:25:33.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/doc/extend.texi	2007-10-24 11:48:18.000000000 +0530
[snip]
> +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
                                ^ Extra ;?

> --- gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-jsrn.c	1970-01-01 05:30:00.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-jsrn.c	2007-10-24 11:55:53.000000000 +0530
> @@ -0,0 +1,17 @@
> +/* Testcase to check generation of a SH2A specific instruction for
> +   "JSR/N @Rm"  */

Missing period.

> +/* { dg-skip-if "" { "sh*-*-*" } "*" "-m2a -m2a-nofpu -m2a-single
> +   -m2a-single-only" }  */

A single line would be safer.

> --- gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-resbank.c	1970-01-01 05:30:00.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-resbank.c	2007-10-24 11:48:18.000000000 +0530
> @@ -0,0 +1,14 @@
> + /* Test for resbank attribute.  */
> + /* { dg-do assemble {target sh*-*-*}} */
> + /* { dg-skip-if "" { "sh*-*-*" } "*" "-m2a -m2a-nofpu -m2a-single -m2a-single-only" } */
> + /* { dg-final { scan-assembler "resbank" } } */
> + 
> + 
> + extern void bar(void);
> + 
> + void foo(void) __attribute__((interrupt_handler, resbank));
> + void foo(void)
> + {
> +  bar();
> + }
> +

Extra space at the head of each lines.

> --- gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-rtsn.c	1970-01-01 05:30:00.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-rtsn.c	2007-10-24 12:03:50.000000000 +0530
> @@ -0,0 +1,13 @@
> +/* Testcase to check generation of a SH2A specific instruction for
> +   "RTS/N"  */

Missing period.

> --- gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-tbr-jump.c	1970-01-01 05:30:00.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-tbr-jump.c	2007-10-24 11:48:18.000000000 +0530
> @@ -0,0 +1,25 @@
> +/* Testcase to check generation of a SH2A specific,
> +   TBR relative jump instruction - "JSR @@(disp8,TBR)"  */

Missing period.

> 2007-10-18	Anil Paranjape <anil.paranjape@kpitcummins.com>
>           	Jayant Sonar <Jayant.sonar@kpitcummins.com>

should be

200z-xx-yy  Anil Paranjape  <anil.paranjape@kpitcummins.com>
	    Jayant Sonar  <Jayant.sonar@kpitcummins.com>

> 	(print_operand): "@" prints resbank if the instruction RESBANK 
> 	has to be generated.

 	(print_operand): Handle resbank in %@ operand code.

would be better.

> 	(push_regs): Add conditions for resbank.
> 	(sh_expand_epilogue): Add conditions for resbank.
> 	(sh_insert_attributes): Add resbank attribute.
> 	(sh_attribute_table): Add resbank and function_vector
> 	attributes.

In such case,

 	(push_regs): Add conditions for resbank.
 	(sh_expand_epilogue): Likewise.
 	(sh_insert_attributes): Likewise.
 	(sh_attribute_table): Likewise.

is enough.

> 	* config/sh/sh.md: (calli): Add JSR/N instruction if the
> 	instruction does not have delay slot.

Wrong format.

	* config/sh/sh.md (calli): Emit jsr/n if possible.

is enough.

> 	(calli_pcrel): Add JSR/N instruction if the instruction does 
> 	not have delay slot.
> 	(return_i): Add RTS/N instruction if the instruction does not
> 	have delay slot.

	(calli_pcrel): Emit jsr/n if possible.
	(return_i): Emit rts/n if possible.

is enough.

> 	* config/sh/sh-protos.h : Update as needed.

Extra space before colon.  A little vague description.
	
	* config/sh/sh-protos.h (sh_cfun_resbank_handler_p): Declare.
	(sh2a_get_function_vector_number): Likewise.
	(sh2a_is_function_vector_call): Likewise.

would be better.

> 	* doc/extend.texi (TBR relative addressing): Added description 
> 	for SH2A target.
> 	(resbank): Added description for SH2A target.

should be

	* doc/extend.texi: Document TBR relative addressing of SH2A.
	(resbank): Add description for SH2A.

though the document changes will be looked by appropriate
maintainers.

> 	* testsuite/gcc.target/sh/sh2a-resbank.c : New.
> 	* testsuite/gcc.target/sh/sh2a-tbr-jump.c: New.

New testcases require an entry in gcc/testsuite/ChangeLog:

	* gcc.target/sh/sh2a-resbank.c: New test.
	* gcc.target/sh/sh2a-tbr-jump.c: New test.
	
Regards,
	kaz


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]