Review of the 2nd piece of the machine-independent part of selective scheduler
Vladimir Makarov
vmakarov@redhat.com
Wed Aug 20 19:55:00 GMT 2008
This is review of 2nd piece of machine-independent part of the
selective scheduler.
The most serious mistake is wrong work with POSIX include files and
functions. It should be fixed in anyway bu using macros generated by
configure. See comments for pwd.h, unistd.h, and fcntl.h.
The most frequent pitfall is absence of comments in some places.
>diff -cprNd -x .svn -x .hg trunk/gcc/sel-sched-dump.c
sel-sched-branch/gcc/sel-sched-dump.c
>*** trunk/gcc/sel-sched-dump.c Thu Jan 1 03:00:00 1970
>--- sel-sched-branch/gcc/sel-sched-dump.c Fri Apr 18 13:23:48 2008
...
>+ #include "config.h"
>+ #include "system.h"
>+ #include "coretypes.h"
>+ #include "tm.h"
>+ #include "toplev.h"
>+ #include "rtl.h"
>+ #include "tm_p.h"
>+ #include "hard-reg-set.h"
>+ #include "regs.h"
>+ #include "function.h"
>+ #include "flags.h"
>+ #include "insn-config.h"
>+ #include "insn-attr.h"
>+ #include "params.h"
>+ #include "output.h"
>+ #include "basic-block.h"
>+ #include "cselib.h"
>+ #include "sel-sched-ir.h"
>+ #include "sel-sched-dump.h"
>+
>+ #include "pwd.h"
Please remove this include. I think that is wrong. GCC works on
different systems not only POSIX ones. As I understand you use
getpwuid from pwd.h. You should write code which works without
getpuwid. The code should look like
#if defined(HAVE_GETPWUID) && defined(HAVE_GETEUID)
{
struct passwd *pw = getpwuid (geteuid ());
...
}
#else
Current directory, e.g.
# endif
#endif
>+ #include <fcntl.h>
>+ #include <unistd.h>
File system.h should contain fcntl.h and unistd.h. You should not use
it explicitly because on some systems it can be absent or be placed in
non-standard directory, e.g. <sys/fcntl.h>. So please remove these
two includes.
It is hard me to say what you are using from these files but the code
should parametrized in way analogous to getpwuid.
>+
>+ /* File for dumping statistics into the one directory for
>+ the whole make run. */
>+ static FILE *sel_stat_file = NULL;
>+
>+
>+
>+ /* These variables control high-level pretty printing. */
>+ static int sel_dump_cfg_flags = SEL_DUMP_CFG_FLAGS;
>+ static int sel_debug_cfg_flags = SEL_DUMP_CFG_FLAGS;
>+
>+ /* True when a cfg should be dumped (either flag_sel_sched_dump cfg
^ Missed _
>+ or the below flag is set. */
>+ static bool sel_dump_cfg_p;
...
>+ /* Stores an expression according to which either insns will be
scheduled
>+ using all features of the selective scheduling or the corresponding
>+ code motion will be skipped. */
>+ const char *flag_insn_range = NULL;
>+ static char sel_stat_output_buf[16384];
Please add a comment for sel_stat_output_buf. It would be better to
allocate it dynamically to reuse memory for other passes.
>+
>+
>+ static FILE *sched_dump1;
>+
>+ static void
>+ switch_dump (FILE *to)
>+ {
>+ sched_dump1 = sched_dump;
>+ sched_dump = to;
>+ }
>+
>+ static void
>+ restore_dump (void)
>+ {
>+ sched_dump = sched_dump1;
>+ }
>+
>+ void
>+ print_marker_to_log (void)
>+ {
>+ sel_print ("Marker: %d\n:", sel_dump_cfg_fileno);
>+ }
Comments please for the above functions and sched_dump_1. What happens if
swicth_dump will be called twice. At least you should add assert to
check this.
>+
>+
>+ /* Functions for dumping instructions, av sets, and exprs. */
>+
>+ static int dump_all = 0;
>+ static int dump_insn_rtx_flags = DUMP_INSN_RTX_PATTERN;
>+ static int dump_vinsn_flags = (DUMP_VINSN_INSN_RTX | DUMP_VINSN_TYPE
>+ | DUMP_VINSN_COUNT);
>+ static int debug_vinsn_flags = 1;
>+ static int debug_insn_rtx_flags = 1;
>+ static int dump_expr_flags = DUMP_EXPR_ALL;
>+ static int debug_expr_flags = 1;
>+
>+ /* Controls how an insn should be dumped. It can be changed from
debugger. */
>+ static int dump_insn_flags = (DUMP_INSN_EXPR | DUMP_INSN_SCHED_CYCLE);
>+ static int debug_insn_flags = 1;
>+
Please, comments for the above variables.
...
>+ void
>+ dump_insn_rtx_1 (rtx insn, int flags)
>+ {
Comments for the function, please. The same fo the next 5 functions.
...
>+ void
>+ dump_insn_rtx (rtx insn)
...
>+
>+ void
>+ debug_insn_rtx (rtx insn)
...
>+
>+ void
>+ dump_vinsn_1 (vinsn_t vi, int flags)
...
>+
>+ void
>+ dump_vinsn (vinsn_t vi)
...
>+ void
>+ debug_vinsn (vinsn_t vi)
...
>+
>+ /* Dump EXPR. */
>+ void
>+ dump_expr_1 (expr_t expr, int flags)
Please, add description of `flags'.
...
>+
>+ void
>+ dump_expr (expr_t expr)
...
>+
>+ void
>+ debug_expr (expr_t expr)
...
>+ void
>+ debug_insn (insn_t insn)
Please, add comments to the 3 functions above.
...
+ /* Dumps a hard reg set SET to FILE using PREFIX. */
^ one more space.
+ static void
+ print_hard_reg_set (FILE *file, const char *prefix, HARD_REG_SET set)
...
>+ /* Replace characters in BUF that have special meaning in .dot file. */
>+ void
>+ sel_prepare_string_for_dot_label (char *buf)
>+ {
>+ char specials_from[7][2] = { "<", ">", "{", "|", "}", "\"",
>+ "\n" };
>+ char specials_to[7][3] = { "\\<", "\\>", "\\{", "\\|", "\\}", "\\\"",
>+ "\\l" };
>+ unsigned i;
Please, add static to the arrays. Otherwise, they will be initialized
every call. That is a lot of code.
...
>+
>+ /* Functions callable from a debugger. */
>+ int
>+ insn_uid (rtx insn)
>+ {
>+ return INSN_UID (insn);
>+ }
>+
>+ basic_block
>+ block_for_insn (rtx insn)
>+ {
>+ return BLOCK_FOR_INSN (insn);
>+ }
>+
>+ av_set_t
>+ bb_av_set (basic_block bb)
>+ {
>+ return BB_AV_SET (bb);
>+ }
>+
>+ rtx insn_pattern (rtx insn)
>+ {
>+ return PATTERN (insn);
>+ }
>+
>+ int insn_code (rtx insn)
>+ {
>+ return GET_CODE (insn);
>+ }
>+
>+ bool insn_is_set_p (rtx insn)
>+ {
>+ return GET_CODE (PATTERN (insn)) == SET;
>+ }
>+
>+ int
>+ hard_regno_rename_ok (int i ATTRIBUTE_UNUSED, int j ATTRIBUTE_UNUSED)
>+ {
>+ return HARD_REGNO_RENAME_OK (i, j);
>+ }
>+
I think you should remove the above functions. If getting macro
values is difficult for you in a debugger, you could use -g3 for this.
...
+ /* Dumps av_set AV to stderr. */
^ one more space.
+ void
+ debug_av_set (av_set_t av)
>+ /* The file name is either copied from the SEL_STAT_FILE variable,
or generated
>+ from the current time. If the environment variable is set, then
>+ all the statistics will be written to the single file, otherwise
>+ statistics will be stored in separate files in the ~/sel-stat
directory. */
>+ static void
>+ sel_get_stat_filename (char *buf)
>+ {
>+ struct passwd *rpw = getpwuid (getuid());
>+ char buf2[1024];
>+ char *stat_file_name = getenv ("SEL_STAT_FILE");
>+
>+ strcpy (buf, rpw->pw_dir);
>+ strcat (buf, "/sel-stat");
>+ mkdir (buf, 0775);
>+
>+ if (stat_file_name)
>+ sprintf (buf2, "/%s", stat_file_name);
>+ else
>+ sprintf (buf2, "/stat-%d.txt", (int) time (NULL));
>+
>+ strcat (buf, buf2);
>+ }
>+
Please, read comments for the includes. You also can not use '/' as
directory separator (you should use DIR_SEPARATOR... defined in
system.h). Also you should process mkdir return value to check that
everything is ok.
...
>diff -cprNd -x .svn -x .hg trunk/gcc/sel-sched-dump.h
sel-sched-branch/gcc/sel-sched-dump.h
>*** trunk/gcc/sel-sched-dump.h Thu Jan 1 03:00:00 1970
>--- sel-sched-branch/gcc/sel-sched-dump.h Fri Apr 18 13:23:48 2008
...
>+ #define SEL_DUMP_CFG_BB_NOTES_LIST (4)
>+ /* Dump av_set from the bb_header. */
>+ #define SEL_DUMP_CFG_AV_SET (8)
>+ #define SEL_DUMP_CFG_LV_SET (16)
>+ #define SEL_DUMP_CFG_BB_INSNS (32)
>+ #define SEL_DUMP_CFG_FENCES (64)
>+ #define SEL_DUMP_CFG_INSN_SEQNO (128)
>+ #define SEL_DUMP_CFG_INSN_FLAGS (0)
>+ #define SEL_DUMP_CFG_FUNCTION_NAME (256)
>+ #define SEL_DUMP_CFG_BB_LIVE (512)
>+ #define SEL_DUMP_CFG_BB_LOOP (1024)
Comments for the macros, please.
...
>+ enum _dump_insn_rtx
>+ {
>+ DUMP_INSN_RTX_UID = 2,
>+ DUMP_INSN_RTX_PATTERN = 4,
>+ DUMP_INSN_RTX_BBN = 8,
>+
>+ DUMP_INSN_RTX_ALL = (DUMP_INSN_RTX_UID | DUMP_INSN_RTX_PATTERN
>+ | DUMP_INSN_RTX_BBN)
Comments for the enum and its values, please.
...
>+ enum _dump_idata
>+ {
>+ DUMP_IDATA_TYPE = 2,
>+ DUMP_IDATA_LHS = 4,
>+ DUMP_IDATA_RHS = 8,
>+ DUMP_IDATA_REG_SETS = 16,
>+ DUMP_IDATA_REG_USES = 32,
>+
>+ DUMP_IDATA_ALL = (DUMP_IDATA_TYPE | DUMP_IDATA_LHS | DUMP_IDATA_RHS
>+ | DUMP_IDATA_REG_SETS | DUMP_IDATA_REG_USES)
Comments for the enum and its values, please.
...
>+ enum _dump_vinsn
>+ {
>+ DUMP_VINSN_INSN_RTX = 2,
>+ DUMP_VINSN_TYPE = 4,
>+ DUMP_VINSN_COUNT = 8,
>+ DUMP_VINSN_COST = 16,
>+
>+ DUMP_VINSN_ALL = (DUMP_VINSN_INSN_RTX | DUMP_VINSN_TYPE |
DUMP_VINSN_COUNT
>+ | DUMP_VINSN_COST)
The same.
...
>+ enum _dump_expr
>+ {
>+ DUMP_EXPR_VINSN = 2,
>+ DUMP_EXPR_SPEC = 4,
>+ DUMP_EXPR_PRIORITY = 8,
>+ DUMP_EXPR_SCHED_TIMES = 16,
>+ DUMP_EXPR_SPEC_DONE_DS = 32,
>+ DUMP_EXPR_ORIG_BB = 64,
>+ DUMP_EXPR_USEFULNESS = 128,
>+
>+ DUMP_EXPR_ALL = (DUMP_EXPR_VINSN | DUMP_EXPR_SPEC |
DUMP_EXPR_PRIORITY
>+ | DUMP_EXPR_SCHED_TIMES | DUMP_EXPR_SPEC_DONE_DS
>+ | DUMP_EXPR_ORIG_BB | DUMP_EXPR_USEFULNESS)
The same.
...
>+ /* A enumeration for dumping flags of an insn. */
>+ enum _dump_insn
>+ {
>+ DUMP_INSN_ASM_P = 2,
>+ DUMP_INSN_SCHED_NEXT = 4,
>+ DUMP_INSN_EXPR = 8,
>+ DUMP_INSN_AV = 16,
>+ DUMP_INSN_SEQNO = 32,
>+ DUMP_INSN_AFTER_STALL_P = 64,
>+ DUMP_INSN_SCHED_CYCLE = 128,
>+ DUMP_INSN_UID = 256,
>+ DUMP_INSN_BBN = 512,
>+ DUMP_INSN_PATTERN = 1024,
>+
>+ DUMP_INSN_ALL = (DUMP_INSN_ASM_P | DUMP_INSN_SCHED_NEXT |
DUMP_INSN_EXPR
>+ | DUMP_INSN_AV | DUMP_INSN_SEQNO | DUMP_INSN_AFTER_STALL_P
>+ | DUMP_INSN_SCHED_CYCLE | DUMP_INSN_UID | DUMP_INSN_BBN
>+ | DUMP_INSN_PATTERN)
Comments for the enum values, please.
...
>+
>+ #define sel_print_to_dot(...) \
>+ do { \
>+ int __j = 1 + 2 * snprintf (NULL, 0, __VA_ARGS__); \
>+ char *__s = alloca (__j * sizeof (*__s)); \
>+ snprintf (__s, __j, __VA_ARGS__); \
>+ sel_prepare_string_for_dot_label (__s); \
>+ fprintf (sched_dump, "%s", __s); \
>+ } while (0)
>+
>+ #define sel_print(...) \
>+ do { \
>+ if (sched_dump_to_dot_p) \
>+ sel_print_to_dot (__VA_ARGS__); \
>+ else \
>+ fprintf (sched_dump, __VA_ARGS__); \
>+ } while (0)
Comments for the macros, please.
...
>diff -cprNd -x .svn -x .hg trunk/gcc/sel-sched-ir.c
sel-sched-branch/gcc/sel-sched-ir.c
>*** trunk/gcc/sel-sched-ir.c Thu Jan 1 03:00:00 1970
>--- sel-sched-branch/gcc/sel-sched-ir.c Mon Jun 2 14:35:56 2008
...
>+ /* A regset pool structure. */
>+ static struct
>+ {
>+ regset *v;
>+ int n;
>+ int s;
>+
>+ /* In VV we accumulate all generated regsets so that, when
destucting the
>+ pool, we can compare it with V and check that every regset was
returned
>+ back to pool. */
>+ regset *vv;
>+ int nn;
>+ int ss;
>+
>+ int diff;
Comments for the members, please.
...
>+ /* This represents the nop pool. */
>+ static struct
>+ {
>+ insn_t *v;
>+ int n;
>+ int s;
Comments for the members, please.
...
>+ } nop_pool = { NULL, 0, 0 };
>+
>+ /* Add ORIGINAL_INSN the def list DL honoring CROSSES_CALL. */
>+ void
>+ def_list_add (def_list_t *dl, insn_t original_insn, bool crosses_call)
>+ {
>+ def_t d;
Add a blank line here, please.
>+ _list_add (dl);
>+ d = DEF_LIST_DEF (*dl);
>+
>+ d->orig_insn = original_insn;
>+ d->crosses_call = crosses_call;
>+ }
...
>+ static struct sched_deps_info_def
_advance_deps_context_sched_deps_info =
Please, add a description of _advance_deps_context_sched_deps_info.
...
+ /* Target hooks wrappers.
+ Possibly it would be nice to provide some default implementations for
+ them. */
^ one more space.
+
+ /* Allocate a store for the target context. */
+ static tc_t
+ alloc_target_context (void)
...
>+
>+ /* Merges two fences (filling fields of OLD_FENCE with resulting
values) by
>+ following rules: 1) state, target context and last scheduled insn are
>+ propagated from fallthrough edge if it is available;
>+ 2) deps context and cycle is propagated from more probable edge;
>+ 3) all other fields are set to corresponding constant values. */
Please, describe all parameters.
...
>+ static void
>+ merge_fences (fence_t f, insn_t insn,
>+ state_t state, deps_t dc, void *tc,
>+ rtx last_scheduled_insn, VEC(rtx, gc) *executing_insns,
>+ int *ready_ticks, int ready_ticks_size,
>+ rtx sched_next, int cycle, bool after_stall_p)
>+
...
>+ regset
>+ get_regset_from_pool (void)
>+ {
...
>+
>+ regset
>+ get_clear_regset_from_pool (void)
...
>+
>+ void
>+ return_regset_to_pool (regset rs)
>+ {
...
>+
>+ static int
>+ cmp_v_in_regset_pool (const void *x, const void *xx)
>+ {
...
>+
>+ void
>+ free_regset_pool (void)
>+ {
Please, comments for the above functions. Even if their names look
like self-explaining, it is better to have them.
...
>+
I'd add new page ^L here.
>+ /* Functions to work with nop pools. NOP insns are used as temporary
>+ placeholders of the insns being scheduled to allow correct update of
>+ the data sets. When update is finished, NOPs are deleted. */
>+
>+ /* A vinsn that is used to represent a nop. This vinsn is shared
among all
>+ nops sel-sched generates. */
>+ static vinsn_t nop_vinsn = NULL;
>+
...
>+
>+ /* Skip unspec to support ia64 speculation. Called from
rtx_equal_p_cb. */
>+
Please, add parameter description.
...
>+ static int
>+ skip_unspecs_callback (const_rtx *xx, const_rtx *yy, rtx *nx, rtx* ny)
>+ {
...
>+
>+ /* Callback, called from hash_rtx_cb.
>+ Helps to hash UNSPEC rtx in a correct way to support ia64
speculation. */
>+
one more
space ^
Please, add parameter description.
...
>+ static int
>+ hash_with_unspec_callback (const_rtx x, enum machine_mode mode
ATTRIBUTE_UNUSED,
...
>+ rtx *nx, enum machine_mode* nmode)
>+ /* Initialize vinsn VI for INSN. Only for use from vinsn_create (). */
Please, describe `force_unique_p'.
>+ static void
>+ vinsn_init (vinsn_t vi, insn_t insn, bool force_unique_p)
>+ {
...
>+ /* Insert HASH in a sorted vector pointed to by PVECT, if HASH is
>+ not there already. */
Please, describe all parameters.
>+ void
>+ insert_in_history_vect (VEC (expr_history_def, heap) **pvect,
>+ unsigned uid, enum local_trans_type type,
>+ vinsn_t old_expr_vinsn, vinsn_t new_expr_vinsn,
>+ ds_t spec_ds)
...
>+ /* Update target_available bits when merging exprs TO and FROM. */
>+ static void
>+ update_target_availability (expr_t to, expr_t from, insn_t split_point)
>+ {
Please, describe `split_point'.
...
>+ /* Update speculation bits when merging exprs TO and FROM. */
>+ static void
>+ update_speculative_bits (expr_t to, expr_t from, insn_t split_point)
>+ {
The same.
...
>+
>+ /* Merge bits of FROM expr to TO expr. Vinsns in the exprs should
correlate. */
>+ void
>+ merge_expr (expr_t to, expr_t from, insn_t split_point)
The same.
...
+ /* Try to make EXPR speculative. Return 1 when EXPR's pattern
+ or dependence status have changed, 2 when also the target register
+ became unavailable, 0 if nothing had to be changed. */
^ one more space.
+ int
+ speculate_expr (expr_t expr, ds_t ds)
...
>+
>+ /* Truncate the set so it doesn't grow too much. */
>+ void
>+ av_set_truncate (av_set_t set)
>+ {
>+ int n = 0;
>+ av_set_t prev;
>+
>+ while (set && ++n <= 9)
I'd define macro for 9 and describe it.
...
>+
>+ static struct
>+ {
>+ deps_where_t where;
>+ idata_t id;
>+ bool force_unique_p;
>+ bool force_use_p;
Please, add comments for the members.
>+ } deps_init_id_data;
>+
>+
>+ /* Setup ID for INSN. */
>+ static void
>+ setup_id_for_insn (idata_t id, insn_t insn, bool force_unique_p)
Please, describe `force_unique_p'.
...
>+ static const struct sched_deps_info_def
const_deps_init_id_sched_deps_info =
Please, add description of the constant.
...
>+ static struct sched_deps_info_def deps_init_id_sched_deps_info;
>+
Please, add description of the variable.
...
>+ /* Scan the region and initialize instruction data. */
>+ void
>+ sel_init_global_and_expr (bb_vec_t bbs)
>+ {
>+ {
>+ /* ??? It would be nice to implement push / pop scheme for
sched_infos. */
>+ const struct sched_scan_info_def ssi =
>+ {
>+ NULL, /* extend_bb */
>+ init_global_and_expr_for_bb, /* init_bb */
>+ extend_insn, /* extend_insn */
>+ init_global_and_expr_for_insn /* init_insn */
>+ };
>+
>+ sched_scan (&ssi, bbs, NULL, NULL, NULL);
>+ }
Please, remove extra {}.
...
>+ /* Finish analyzing dependencies of an lhs. */
^ a
>+ static void
>+ has_dependence_finish_lhs (void)
...
>+
>+ /* Start analyzing dependencies of an rhs. */
^ a
>+ static void
>+ has_dependence_finish_rhs (void)
...
>+ static const struct sched_deps_info_def
const_has_dependence_sched_deps_info =
...
>+ static struct sched_deps_info_def has_dependence_sched_deps_info;
Please, descriptions for the constant and variable.
>+
>+ static void
>+ setup_has_dependence_sched_deps_info (void)
>+ {
...
>+
>+ void
>+ sel_clear_has_dependence (void)
Comment for 2 functions above, please.
...
>+
>+ /* Return nonzero if EXPR has is dependent upon PRED. */
Please, describe `has_dep_pp'.
>+ ds_t
>+ has_dependence_p (expr_t expr, insn_t pred, ds_t **has_dep_pp)
>+ {
...
>+ /* Update minimal scheduling cycle for tick_check_insn given that it
depends
>+ on PRO with status DS and weight DW. */
>+ static void
>+ tick_check_dep_with_dw (insn_t pro_insn, ds_t ds, dw_t dw)
>+ {
>+ expr_t con_expr = tick_check_data.expr;
>+ insn_t con_insn = EXPR_INSN_RTX (con_expr);
>+
>+ if (con_insn != pro_insn)
>+ {
>+ enum reg_note dt;
>+ int tick;
>+
>+ if (/* PROducer was removed from above due to pipelining. See
PR8. */
What is PR8? Could you explain more.
...
>+
>+ static struct sched_deps_info_def _tick_check_sched_deps_info =
Please, describe the variable.
...
>+
>+ #ifdef ENABLE_CHECKING
>+ static void
>+ verify_backedges (void)
Please, document the function.
...
>+ /* Rip-off INSN from the insn stream. When ONLY_DISCONNECT is true,
>+ do not delete insn's data, because it will be later re-emitted.
>+ Return true if we have removed some blocks afterwards. */
>+ bool
>+ sel_remove_insn (insn_t insn, bool only_disconnect, bool full_tidying)
Please, describe `full_tidying'.
...
>+
>+ static insn_vec_t new_insns = NULL;
Please, document the variable
...
>+
>+ static void
>+ copy_lv_set_from (basic_block bb, basic_block from_bb)
>+ {
...
>+
>+ av_set_t
>+ get_av_set (insn_t insn)
>+ {
Please, document the 2 functions above.
...
>+ /* A pool for allocating successor infos. */
>+ static struct
>+ {
>+ struct succs_info *stack;
>+ int size, top, max_top;
Please, document the structure members.
...
>+
>+ /* Same as above, but fill PROBS vector with probabilities of
corresponding
>+ successors depending on INSN. */
>+ struct succs_info *
>+ compute_succs_info (insn_t insn, short flags)
>+ {
Please, document `flags'.
...
>+ /* Returns the basic block to which jump instruction JUMP makes a
jump (in case
>+ it does not have several destinations). */
>+ static inline basic_block
>+ jump_destination (insn_t jump)
>+ {
>+ basic_block bb = BLOCK_FOR_INSN (jump);
Add a blank line here.
>+ gcc_assert (JUMP_P (jump) && BB_END (BLOCK_FOR_INSN (jump)) == jump);
>+ if (EDGE_COUNT (bb->succs) > 2)
>+ return NULL;
>+ if (EDGE_COUNT (bb->succs) == 1)
>+ return EDGE_SUCC (bb, 0)->dest;
>+ if (EDGE_SUCC (bb, 0)->flags & EDGE_FALLTHRU)
>+ return EDGE_SUCC (bb, 1)->dest;
>+ return EDGE_SUCC (bb, 0)->dest;
>+ }
>+
...
>+
>+ #if defined ENABLE_RTL_CHECKING
>+ /* This check is verifies that all jumps jump where they should.
^ a typo.
...
>+ /* Remove an empty basic block EMPTY_BB. When MERGE_UP_P is true,
we put
>+ EMPTY_BB's note lists into its predecessor instead of putting them
>+ into the successor. */
>+ void
>+ sel_remove_empty_bb (basic_block empty_bb, bool merge_up_p,
>+ bool remove_from_cfg_p)
Please, document `remove_from_cfg_p'
...
>+ static struct cfg_hooks orig_cfg_hooks;
>+
Add a comment for the variable, please.
...
>+ /* Update the latch when we've splitted or merged it.
>+ This should be checked for all outer loops, too. */
>+ static void
>+ change_loops_latches (basic_block from, basic_block to)
Document the parameters, please.
...
>+
>+ /* Implement sched_create_recovery_block (). */
>+ basic_block
>+ sel_create_recovery_block (insn_t orig_insn)
Document parameter, please.
...
>+ static struct cfg_hooks sel_cfg_hooks;
>+
Add a comment for the variable, please.
...
>+ /* Emit an insn rtx based on PATTERN. */
Document `label', please.
>+ static rtx
>+ create_insn_rtx_from_pattern_1 (rtx pattern, rtx label)
>+ {
...
>+
>+ /* Emit an insn rtx based on PATTERN and ICE if the result is not a
valid
>+ insn. */
>+ rtx
>+ create_insn_rtx_from_pattern (rtx pattern, rtx label)
>+ {
Ditto.
...
>+ /* Create a new vinsn for INSN_RTX. */
>+ vinsn_t
>+ create_vinsn_from_insn_rtx (rtx insn_rtx, bool force_unique_p)
>+ {
Document `force_unique_p', please.
...
>+ /* Setup special insns used in the scheduler. */
>+ void
>+ setup_nop_and_exit_insns (void)
>+ {
>+ gcc_assert (nop_pattern == NULL_RTX
>+ && exit_insn == NULL_RTX);
>+
>+ nop_pattern = gen_nop ();
>+
>+ {
>+ start_sequence ();
>+ insn_init.what = INSN_INIT_WHAT_INSN_RTX;
>+ emit_insn (nop_pattern);
>+ exit_insn = get_insns ();
>+ end_sequence ();
>+ set_block_for_insn (exit_insn, EXIT_BLOCK_PTR);
>+ }
Please, remove extra {};
...
>+
Add new page ^L here, please.
...
>+ /* If BB1 has a smaller topological sort number than BB2, returns -1;
^ basic block X ^ basic block Y
>+ if greater, returns 1. */
>+ static int
>+ bb_top_order_comparator (const void *x, const void *y)
>+ {
...
>diff -cprNd -x .svn -x .hg trunk/gcc/sel-sched-ir.h
sel-sched-branch/gcc/sel-sched-ir.h
>*** trunk/gcc/sel-sched-ir.h Thu Jan 1 03:00:00 1970
>--- sel-sched-branch/gcc/sel-sched-ir.h Thu May 29 18:28:30 2008
...
>+ /* Expression information. */
>+ struct _expr
>+ {
>+ /* Insn description. */
>+ vinsn_t vinsn;
>+
>+ /* SPEC is the degree of speculativeness.
>+ FIXME: now spec is increased when an rhs is moved through a
>+ conditional, thus showing only control speculativeness. In the
>+ future we'd like to count data spec separately to allow a better
>+ control on scheduling. */
>+ int spec;
>+
>+ /* Degree of speculativeness too. Shows the chance of the result of
>+ instruction to be actually used if it is moved to the current
point. */
Please, add comment about measurement of the degree.
...
>+ /* Availability sets arae sets of expressions we're scheduling. */
^ typo
>+ typedef _list_t av_set_t;
>+ #define _AV_SET_EXPR(L) (&(L)->u.expr)
>+ #define _AV_SET_NEXT(L) (_LIST_NEXT (L))
>+
>+
>+ /* Scheduling boundary. This now corresponds to fence as we finish
a cycle
>+ at the end of bb. */
>+ struct _bnd
>+ {
>+ insn_t to;
>+ ilist_t ptr;
>+ av_set_t av;
>+ av_set_t av1;
Please, document the above four members.
...
>+ /* List iterator backend. */
>+ typedef struct
>+ {
>+ _list_t *lp;
>+ bool can_remove_p;
>+ bool removed_p;
Please, document the memebers.
...
>+ /* General macros to traverse a list. FOR_EACH_* interfaces are
>+ implemented using these. */
>+ #define _FOR_EACH(TYPE, ELEM, I, L) \
>+ for (_list_iter_start (&(I), &(L), false); \
>+ _list_iter_cond_##TYPE (*(I).lp, &(ELEM)); \
>+ _list_iter_next (&(I)))
>+
>+ #define _FOR_EACH_1(TYPE, ELEM, I, LP) \
>+ for (_list_iter_start (&(I), (LP), true); \
>+ _list_iter_cond_##TYPE (*(I).lp, &(ELEM)); \
>+ _list_iter_next (&(I)))
Could you define it as
#define _FOR_EACH_1(TYPE, ELEM, I, LP) _FOR_EACH(TYPE, ELEM, I, *(LP))
...
>+
>+ /* InstructionData. Contains information about insn pattern. */
>+ struct idata_def
>+ {
...
>+ /* Registers that are set/used by this insn. This info is now
gathered
^ set/cloberred/used
>+ via sched-deps.c. The downside of this is that we also use
live info
>+ from flow that is accumulated in the basic blocks. These two infos
>+ can be slightly inconsistent, hence in the beginning we make a pass
>+ through CFG and calculating the conservative solution for the
info in
>+ basic blocks. When this scheduler will be switched to use
dataflow,
>+ this can be unified as df gives us both per basic block and per
>+ instruction info. Actually, we don't do that pass and just hope
>+ for the best. */
>+ regset reg_sets;
>+
>+ regset reg_clobbers;
>+
>+ regset reg_uses;
>+
>+ /* I hope that our renaming infrastructure handles this. */
>+ /* bool has_internal_dep; */
Could you remove the above.
...
>+ struct _sel_insn_data
>+ {
>+ /* Every insn in the stream has an associated vinsn. This is used
>+ to reduce memory consumption and give use the fact that many insns
^ typo
>+ (all of them at the moment) don't change through the scheduler.
>+
>+ Much of the information, that reflect information about the
insn itself
>+ (e.g. not about where it stands in CFG) is contained in the
VINSN_ID
>+ field of this home VI. */
...
>+ /* Return true is INSN is a local NOP. The nop is local in the
sense that
^ if
>+ it was emitted by the scheduler as a temporary insn and will soon be
>+ deleted. These nops are identified by their pattern. */
>+ #define INSN_NOP_P(INSN) (PATTERN (INSN) == nop_pattern)
>+
>+ /* Return true if INSN is linked into instruction stream.
>+ NB: It is impossible for INSN to have one field null and the
other not
>+ null: gcc_assert ((PREV_INSN (INSN) == NULL_RTX)
>+ == (NEXT_INSN (INSN) == NULL_RTX)) is valid. */
>+ #define INSN_IN_STREAM_P(INSN) (PREV_INSN (INSN) && NEXT_INSN (INSN))
>+
>+ /* Return true in INSN is in current fence. */
^ if
>+ /* Types used when initializing insn data. */
>+
>+ enum insn_init_what { INSN_INIT_WHAT_INSN, INSN_INIT_WHAT_INSN_RTX };
Wrong formating. Document the enum values.
...
>+ /* Successor iterator backend. */
>+ typedef struct
>+ {
>+ bool bb_end;
>+ edge e1;
>+ edge e2;
>+ edge_iterator ei;
>+ basic_block bb;
>+
Please, document the members above.
...
>+ /* Collect all loop exits recursively, skipping empty BBs between
them.
>+ E.g. if BB is a loop header which has several loop exits,
>+ traverse all of them and if any of them turns out to be another
loop header
>+ (after skipping empty BBs), add its loop exits to the resulting
vector
>+ as well. */
>+ static inline VEC(edge, heap) *
>+ get_all_loop_exits (basic_block bb)
>+ {
>+ VEC(edge, heap) *exits = NULL;
>+
>+ /* If bb is empty, and we're skipping to loop exits, then
>+ consider bb as a possible gate to the inner loop now. */
>+ while (sel_bb_empty_p (bb)
>+ && in_current_region_p (bb))
>+ {
>+ bb = single_succ (bb);
>+
>+ /* This empty block could only lead outside the region. */
>+ gcc_assert (! in_current_region_p (bb));
>+ }
>+
>+ /* And now check whether we should skip over inner loop. */
>+ if (inner_loop_header_p (bb))
>+ {
>+ struct loop *this_loop;
>+ int i;
>+ edge e;
>+
>+ {
>+ struct loop *pred_loop = NULL;
>+
>+ for (this_loop = bb->loop_father;
>+ this_loop && this_loop != current_loop_nest;
>+ this_loop = loop_outer (this_loop))
>+ pred_loop = this_loop;
>+
>+ this_loop = pred_loop;
>+
>+ gcc_assert (this_loop != NULL);
>+ }
Please, remove extra {}.
...
More information about the Gcc-patches
mailing list