Re: [PATCH - updated] New Optimization: Partitioning hot & coldbasic blocks

[Andreas Jaeger <aj at suse dot de>]

Caroline Tice <ctice@apple.com> writes:

Just some short comments on some things I noticed while glancing over
the patch:

> [...]
> Index: gcc/cfgcleanup.c
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/cfgcleanup.c,v
> retrieving revision 1.95
> diff -c -3 -p -r1.95 cfgcleanup.c
> *** gcc/cfgcleanup.c	16 Sep 2003 21:14:41 -0000	1.95
> --- gcc/cfgcleanup.c	9 Oct 2003 22:47:14 -0000
> *************** static bool mark_effect (rtx, bitmap);
> *** 85,90 ****
> --- 85,92 ----
>   static void notice_new_block (basic_block);
>   static void update_forwarder_flag (basic_block);
>   static int mentions_nonequal_regs (rtx *, void *);
> + bool has_section_boundary_note (basic_block);
> + bool has_dont_shorten_note (basic_block);

Either make these static or move the out of the file in a header file.

> + bool
> + has_dont_shorten_note (bb)
> +      basic_block bb;

Please use ISO C90 prototypes and do not introduce again K&R ones.

> Index: gcc/cfglayout.c
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/cfglayout.c,v
> retrieving revision 1.48
> diff -c -3 -p -r1.48 cfglayout.c
> *** gcc/cfglayout.c	5 Oct 2003 13:09:48 -0000	1.48
> --- gcc/cfglayout.c	9 Oct 2003 22:47:14 -0000
> *************** static void fixup_fallthru_exit_predeces
> *** 58,63 ****
> --- 58,66 ----
>   static rtx duplicate_insn_chain (rtx, rtx);
>   static void break_superblocks (void);
>   static tree insn_scope (rtx);
> + 
> + static void update_unlikely_executed_notes (basic_block);
> + extern bool has_dont_shorten_branch (basic_block);

Please put the extern in an appropriate header file, it should not
live here.

> Index: gcc/cfglayout.h
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/cfglayout.h,v
> retrieving revision 1.9
> diff -c -3 -p -r1.9 cfglayout.h
> *** gcc/cfglayout.h	3 Jul 2003 23:50:05 -0000	1.9
> --- gcc/cfglayout.h	9 Oct 2003 22:47:14 -0000
> *************** extern bool can_copy_bbs_p (basic_block 
> *** 45,47 ****
> --- 45,50 ----
>   extern void copy_bbs (basic_block *, unsigned, basic_block *,
>   		      edge *, unsigned, edge *, struct loop *, struct loops *);
>   extern void cfg_layout_initialize_rbi	(basic_block);
> + extern bool has_section_boundary_note (basic_block);
> + extern bool has_dont_shorten_note (basic_block);
> + extern bool scan_ahead_for_unlikely_executed_note (rtx);

You've got the extern declarations here already, so no need for them
above - or why do you duplicate them?

> Index: gcc/common.opt
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/common.opt,v
> retrieving revision 1.16
> diff -c -3 -p -r1.16 common.opt
> *** gcc/common.opt	3 Sep 2003 20:57:31 -0000	1.16
> --- gcc/common.opt	9 Oct 2003 22:47:14 -0000
> *************** frename-registers
> *** 536,541 ****
> --- 536,545 ----
>   Common
>   Perform a register renaming optimization pass
>   
> + freorder-blocks-and-partition
> + Common
> + Reorder basic blocks and parition into hot and cold sections

Typo: partition


> 	* config/rs6000/rs6000.h (LONG_COND_BRANCH_SIZE):  Added new
> 	* definition.
> 	* doc/invoke.texi  (freorder-blocks-and-partition): Added
> 	* documentation for this new flag.
> 	* doc/tm.text (SECTION_FORMAT_STRING): Added documentation for
> 	* this new macro.

You've got some extra "*" here,

Andreas
-- 
 Andreas Jaeger, aj@suse.de, http://www.suse.de/~aj
  SuSE Linux AG, Deutschherrnstr. 15-19, 90429 Nürnberg, Germany
   GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126

Attachment: pgp00000.pgp
Description: PGP signature