On Thu, Jul 14, 2011 at 5:00 PM, Ilya Enkovich <enkovich....@gmail.com> wrote: >> 2011/7/14 Richard Guenther <richard.guent...@gmail.com>: >>> >>> But then how comes the option to override it is useful? It isn't dependent >>> on the particular case. At least the option should be a --param. >>> >>> Richard. >>> >> >> Option is still useful if you want to try feature on platform with no >> hook implemented and for other performance experiments. I agree >> --param usage should be better here. I'll fix it. >> >> Ilya >> > > Here is a patch with new param instead of new option. Bootstrapped and > checked on x86_64-linux.
} +static int +ix86_reassociation_width (const_gimple stmt) all functions need comments describing what they do and what parameters they get. +@deftypefn {Target Hook} int TARGET_SCHED_REASSOCIATION_WIDTH (const_gimple @var{stmt}) +This hook is called by tree reassociator to determine a level of +parallelism required in output calculations chain. I don't think we should get an actual statement here, but an enum tree_code and a machine mode. +/* Find out how many cycles we need to compute whole statements + chain holding ops_num operands if may execute up to cpu_width + statements per cycle. */ I have a hard time parsing this sentence, maybe a native english speaker can help here. +static int +get_reassociation_width (VEC(operand_entry_t, heap) * ops, gimple stmt) +{ + int param_width = PARAM_VALUE (PARAM_TREE_REASSOC_WIDTH); + int ops_num = VEC_length (operand_entry_t, ops); + int width; + int cycles_best; + + if (param_width > 0) + width = param_width; + else + width = targetm.sched.reassociation_width (stmt); this is the only place you need stmt, with tree-code and mode args you'd not need it (and it's odd anyway). + while (width > 1 && + get_required_cycles (ops_num, width - 1) == cycles_best) &&s go on the next line, similar cases in your patch as well + if (dump_file && (dump_flags & TDF_DETAILS)) + { + fprintf (dump_file, + "Width = %d was chosen for reassociation\n", width); + } no {}s around single-stmts +static void +rewrite_expr_tree_parallel (gimple stmt, int width, + VEC(operand_entry_t, heap) * ops) +{ it's not easy to follow the flow of this function, esp. I wonder + else + { + tree var = create_tmp_reg (TREE_TYPE (last_rhs1), "reassoc"); + add_referenced_var (var); + stmts[i] = build_and_add_sum (var, op1, op2, opcode); + } what you need to create new stmts for, and if you possibly create multiple temporary variables here. You don't seem to handle the special-cases of rewrite_expr_tree for the leafs of your tree, especially the PHI node special-casing. I think you may run into vectorization issues here. - TODO_verify_ssa + TODO_remove_unused_locals + | TODO_verify_ssa why? I think the patch looks reasonable overall, please update it with the above comments and re-post it. Thanks, Richard. > Ilya > -- > gcc/ > > 2011-07-14 Enkovich Ilya <ilya.enkov...@intel.com> > > PR middle-end/44382 > * target.def (reassociation_width): New hook. > > * doc/tm.texi.in (reassociation_width): New hook documentation. > > * doc/tm.texi (reassociation_width): Likewise. > > * doc/invoke.texi (tree-reassoc-width): New param documented. > > * hooks.h (hook_int_const_gimple_1): New default hook. > > * hooks.c (hook_int_const_gimple_1): Likewise. > > * config/i386/i386.h (ix86_tune_indices): Add > X86_TUNE_REASSOC_INT_TO_PARALLEL and > X86_TUNE_REASSOC_FP_TO_PARALLEL. > > (TARGET_REASSOC_INT_TO_PARALLEL): New. > (TARGET_REASSOC_FP_TO_PARALLEL): Likewise. > > * config/i386/i386.c (initial_ix86_tune_features): Add > X86_TUNE_REASSOC_INT_TO_PARALLEL and > X86_TUNE_REASSOC_FP_TO_PARALLEL. > > (ix86_reassociation_width) implementation of > new hook for i386 target. > > * params.def (PARAM_TREE_REASSOC_WIDTH): New param added. > > * tree-ssa-reassoc.c (get_required_cycles): New function. > (get_reassociation_width): Likewise. > (rewrite_expr_tree_parallel): Likewise. > > (reassociate_bb): Now checks reassociation width to be used > and call rewrite_expr_tree_parallel instead of rewrite_expr_tree > if needed. > > (pass_reassoc): TODO_remove_unused_locals flag added. > > gcc/testsuite/ > > 2011-07-14 Enkovich Ilya <ilya.enkov...@intel.com> > > * gcc.dg/tree-ssa/pr38533.c (dg-options): Added option > --param tree-reassoc-width=1. > > * gcc.dg/tree-ssa/reassoc-24.c: New test. > * gcc.dg/tree-ssa/reassoc-25.c: Likewise. >