Hi!
On Wed, Jul 28, 2021 at 10:59:50AM +0800, Kewen.Lin wrote:
> >> +/* As a visitor function for each statement cost entry handled in
> >> + function add_stmt_cost, gather some information and update its
> >> + relevant fields in target cost accordingly. */
> >
> > I got lost trying to parse that.. (could be just me :-)
> >
> > Possibly instead something like
> > /* Helper function for add_stmt_cost ; gather information and update
> > the target_cost fields accordingly. */
>
> OK, will update. I was thinking for each entry handled in function
> add_stmt_cost, this helper acts like a visitor, trying to visit each
> entry and take some actions if some conditions are satisifed.
It (thankfully!) has nothing to do with the "visitor pattern", so some
other name might be better :-)
> > Maybe clearer to read if you rearrange slightly and flatten it ? I
> > defer to others on that..
> >
> > if ((kind == vec_to_scalar
> > || kind == vec_perm
> > || kind == vec_promote_demote
> > || kind == vec_construct
> > || kind == scalar_to_vec)
> > || (kind == vector_stmt && where == vect_body)
>
> This hunk is factored out from function rs6000_add_stmt_cost, maybe I
> can keep the original formatting? The formatting tool isn't so smart,
> and sometimes rearrange things to become unexpected (although it meets
> the basic rule, not so elegant), sigh.
It has too many parens, making grouping where there is none, that is the
core issue.
if (kind == vec_to_scalar
|| kind == vec_perm
|| kind == vec_promote_demote
|| kind == vec_construct
|| kind == scalar_to_vec
|| (kind == vector_stmt && where == vect_body))
Segher
