Re: [PATCH v2] rs6000: Add load density heuristic

[Segher Boessenkool <segher@kernel.crashing.org>]

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