Re: [PATCH Atom][PR middle-end/44382] Tree reassociation improvement

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Jul 14, 2011 at 5:00 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2011/7/14 Richard Guenther <richard.guenther@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.enkovich@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.enkovich@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.
>