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

[Dorit Nuzman <DORIT at il dot ibm dot com>]

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

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]