[PATCH] support to flip modes in sh mode-switching

Kaz Kojima kkojima@rr.iij4u.or.jp
Sun Dec 17 13:52:00 GMT 2006


Christian BRUEL <christian.bruel@st.com> 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.  */
> +void
> +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
  http://gcc.gnu.org/contribute.html#docchanges
too.

> +/* 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
non-trivial.

> +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.

Regards,
	kaz



More information about the Gcc-patches mailing list