This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] support to flip modes in sh mode-switching
- From: Kaz Kojima <kkojima at rr dot iij4u dot or dot jp>
- To: christian dot bruel at st dot com
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Sun, 17 Dec 2006 22:52:24 +0900 (JST)
- Subject: Re: [PATCH] support to flip modes in sh mode-switching
- References: <4582CC98.email@example.com>
Christian BRUEL <firstname.lastname@example.org> wrote:
> This patch allows to switch mode (instead of setting it) on
> architectures providing a modes-switching instruction, such as 'fpchg'
> on sh4-300.
I can't review your patch, but it seems to me the patch does
the right thing. I have a few comments, though.
> validated it on sh4-300 with gcc testsuite.
The patch should be tested with bootstrap and the regression test
also on the primary target including a bootstrappable x86 target
at least, since it touches the generic and the i386 specific files.
Also any gcc patch requires its ChangeLog entry and it looks your
patch has some minor problems with gcc coding standards.
> +/* This function flip the fpscr
> + MODE is the mode we are setting it to. */
> +emit_fpu_flip ()
^ "void" needed here
Also each sentence in comments should end with a period.
> + emit_insn (gen_toggle_pr());
^ a space needed here
It looks there are another missing spaces between function-name
and open paren.
> -#define EMIT_MODE_SET(ENTITY, MODE, HARD_REGS_LIVE) \
> - fpscr_set_from_mem ((MODE), (HARD_REGS_LIVE))
> +#define EMIT_MODE_SET(ENTITY, MODE, ACTUALMODE, HARD_REGS_LIVE) \
> + ((TARGET_SH4A_FP || TARGET_SH4_300) \
> + && (ACTUALMODE) != -1 \
> + ? emit_fpu_flip() \
^ a space needed here
This change of EMIT_MODE_SET requires the corresponding change
of the description of EMIT_MODE_SET in gcc/doc/tm.texi. About
documentation changes, please check
> +/* we need to wait before EMIT_MODE_SET to know if all incoming edges have a
> + known edge values or not. Real mode creation is done in commit_mode_set.
> + eg->aux2 is used to hold the mode value for avin. */
The first sentence should start with a capital letter and two spaces
are required between the period and the next sentence. The individual
new function requires a comment especially when that function is
> +set_edge_modes_internal(basic_block bb, sbitmap visited, struct bb_info *info, int j, int no_mode)
Missing space before "(". Also the line should be wrapped so it
is less than ~75 characters on a line. It looks there are another
lines beyond this limit.