Re: [patch] Loop-aware SLP 4/5

[Ira Rosen <IRAR at il dot ibm dot com>]

Dorit Nuzman/Haifa/IBM wrote on 09/09/2007 11:00:13:

> Ira Rosen/Haifa/IBM wrote on 09/09/2007 10:12:40:
>
> > Dorit Nuzman/Haifa/IBM wrote on 18/08/2007 21:36:27:
> >
> > > Ira Rosen/Haifa/IBM wrote on 14/08/2007 16:15:19:
> > >
> > > > This part adds an adjustment of vectorization cost model to SLP.
> > > >
> > > > SLP costs are calculated for each SLP computation tree recursively
> > > > during its analysis, and added to the overall costs if we decide to
SLP.
> > > >
> > > > Thanks,
> > > > Ira
> > > >
> > >
> > > +/* SLP costs are calculated according to SLP instance unrolling
> > factor (i.e.,
> > > +   the number of created vector stmts depends on the unrolling
> > > factor). However,
> > > +   the actual number of vector stmts for every SLP node depends on
> > > VF which is
> > > +   set later in vect_analyze_operations(). Hence, SLP costs should
> > > be updated.
> > > +   In this function we assume that the inside costs calculated in
> > > +   vect_model_xxx_cost are linear in ncopies.  */
> > >
> > > can you explain why this assumption holds (or when it may be broken,
> > > and assert against that)?
> > >
> > > +      /* We assume that costs are linear in ncopies.  */
> > > +      if (SLP_INSTANCE_UNROLLING_FACTOR (instance) != vf)
> > > + SLP_INSTANCE_INSIDE_OF_LOOP_COST (instance) *= vf
> > > +   / SLP_INSTANCE_UNROLLING_FACTOR (instance);
> > >
> > > to me it's less confusing if you remove the 'if' (when unrolling==vf
> > > you'll just multiply by 1, so no need for the special check).
> > >
> >
> > Done.
> >
> > > and lastly, about the following repeating change:
> > >
> > > +  int *inside_cost_field, *outside_cost_field;
> > > +
> > > +  /* Take addresses of relevant fields to update in the function.
*/
> > > +  if (slp_node)
> > > +    {
> > > +      inside_cost_field = &(SLP_TREE_INSIDE_OF_LOOP_COST
(slp_node));
> > > +      outside_cost_field = &(SLP_TREE_OUTSIDE_OF_LOOP_COST
(slp_node));
> > > +    }
> > > +  else
> > > +    {
> > > +      inside_cost_field = &(STMT_VINFO_INSIDE_OF_LOOP_COST
(stmt_info));
> > > +      outside_cost_field = &(STMT_VINFO_OUTSIDE_OF_LOOP_COST
> (stmt_info));
> > > +    }
> > > ...
> > > -        STMT_VINFO_OUTSIDE_OF_LOOP_COST (stmt_info) = outer_cost;
> > > +        *outside_cost_field = outer_cost;
> > >
> > >          inner_cost += ncopies * (TARG_VEC_LOAD_COST +
> TARG_VEC_STMT_COST);
> > >
> > > @@ -607,12 +654,12 @@ vect_model_load_cost (stmt_vec_info stmt
> > >        gcc_unreachable ();
> > >      }
> > >
> > > -  STMT_VINFO_INSIDE_OF_LOOP_COST (stmt_info) = inner_cost;
> > > +  *inside_cost_field = inner_cost;
> > >
> > > Again, in an attempt to reduce "if (slp_node)" switches in the code,
> > > I'd replace
> > >  STMT_VINFO_INSIDE_OF_LOOP_COST (stmt_info) = cost;
> > >  STMT_VINFO_OUTSIDE_OF_LOOP_COST (stmt_info) = cost;
> > > with
> > >  stmt_vinfo_inside_of_loop_cost (stmt_info, slp_node, cost);
> > >  stmt_vinfo_outside_of_loop_cost (stmt_info, slp_node, cost);
> > >
> > > ...and hide the "if (slp_node)" switch there:
> > >  stmt_vinfo_inside_of_loop_cost (stmt_info, slp_node, cost)
> > >    {
> > >      if (slp_node)
> > >   SLP_TREE_INSIDE_OF_LOOP_COST (slp_node) = cost;
> > >      else
> > >   STMT_VINFO_INSIDE_OF_LOOP_COST (stmt_info) = cost;
> > >    }
> > >
> > > (for your consideration...)
> >
> > I moved the choice of the relevant field to a different (inlined)
function.
> >
>
> I prefer the alternative I suggested above - it's a bit less code
> (no need for the call to the selection function) and more
> importantly the whole field selection machinery is entirely hidden.
> Please consider changing (or explain why you prefer the other solution).
>
> In the meantime, ok to commit this patch and all the other parts as
> well (the above change can be applied separately later).

O.K. I will reconsider it.
Committed as is meantime.

Thanks,
Ira


>
> thanks for addressing the comments,
>
> dorit
>
> > Thanks,
> > Ira
> >
> > >
> > > ok otherwise,
> > >
> > > thanks,
> > > dorit
> > >
> > > > ChangeLog:
> > > >
> > > >  * tree-vectorizer.h (_slp_tree): Add new fields for costs and
their
> > > >  access functions.
> > > >  (_slp_instance): Likewise.
> > > >  (vect_model_simple_cost, vect_model_store_cost,
vect_model_load_cost):
> > > >  Declare (make extern).
> > > >  * tree-vect-analyze.c (vect_update_slp_costs_according_to_vf):
New.
> > > >  (vect_analyze_operations): Call
vect_update_slp_costs_according_to_vf.
> > > >  (vect_get_and_check_slp_defs): Calculate costs. Add arguments.
> > > >  (vect_build_slp_tree): Likewise.
> > > >  (vect_analyze_slp_instance): Initialize cost fields. Update
> > > >  arguments of vect_build_slp_tree.
> > > >  * tree-vect-transform.c (vect_estimate_min_profitable_iters): Take
> > > >  SLP costs into account.
> > > >  (vect_model_simple_cost): Make extern, add SLP parameter and
handle
> > > >  SLP.
> > > >  (vect_model_store_cost, vect_model_load_cost): Likewise.
> > > >       (vectorizable_call): Add argument to vect_model_simple_cost.
> > > >       (vectorizable_assignment): Call vect_model_simple_cost
> only for not
> > > >       pure SLP stmts.
> > > >       (vectorizable_operation): Likewise.
> > > >       (vectorizable_type_demotion): Add argument to
> > > >  vect_model_simple_cost.
> > > >       (vectorizable_type_promotion): Likewise.
> > > >       (vectorizable_store): Call vect_model_simple_cost only
> for not pure
> > > >  SLP stmts.
> > > >       (vectorizable_load): Likewise.
> > > >
> > > > [attachment "slp-part4.txt" deleted by Dorit Nuzman/Haifa/IBM]