This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH,SH] Add SH2A new instructions 1/6
- From: Kaz Kojima <kkojima at rr dot iij4u dot or dot jp>
- To: naveen dot hs at kpitcummins dot com
- Cc: gcc-patches at gcc dot gnu dot org, Anil dot Paranjape at kpitcummins dot com, Prafulla dot Thakare at kpitcummins dot com
- Date: Mon, 29 Oct 2007 08:35:12 +0900 (JST)
- Subject: Re: [PATCH,SH] Add SH2A new instructions 1/6
- References: <7B5265FF85A78C4BB9DAD0EE699C47600336E70E@sohm.kpit.com>
"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