On Thu, Jul 14, 2011 at 5:00 PM, Ilya Enkovich <[email protected]> wrote:
>> 2011/7/14 Richard Guenther <[email protected]>:
>>>
>>> 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 <[email protected]>
>
> 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 <[email protected]>
>
> * 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.
>
