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

Reply via email to