Re: [PATCH, RFC, rs6000, v2] folding of vec_splat

[Will Schmidt <will_schmidt@vnet.ibm.com>]

On Thu, 2018-08-16 at 15:51 -0500, Segher Boessenkool wrote:
> Hi Will,
> 
> On Thu, Aug 16, 2018 at 10:50:45AM -0500, Will Schmidt wrote:
> > 2018-08-16  Will Schmidt  <will_schmidt@vnet.ibm.com>
> > 
> > 	* config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Add support for
> > 	  early gimple folding of vec_splat().
> 
> Continuation lines should be indented to the *, not to the text after it.
OK

> 
> > +	/* Only fold the vec_splat_*() if arg1 is a constant
> > +	   5-bit unsigned literal.  */
> > +	if (TREE_CODE (arg1) != INTEGER_CST || TREE_INT_CST_LOW (arg1) > 0x1f)
> > +	  return false;
> 
> Does this need to check for negative as well?  So something with IN_RANGE
> then.

> 
> > +	/* force arg1 into range.  */
> > +	tree new_arg1 = build_int_cst (arg1_type,
> > +				       TREE_INT_CST_LOW (arg1) % n_elts);
> 
> Or is the range check unnecessary completely, since you have this?

I can be convinced either way. :-)
I think i still want both.  The first rejects (from gimple-folding) any
values that are outside of the 5-bit range as specified by the function
definition.
The second (modulo) then maps any 'valid' values into what is reasonable
for the data type being splatted.

While trying to convince myself one way or another, I do notice that
today (pre-folding), i can get out-of-range errors such as 
  /tmp/ccP0s6iJ.s:781: Error: operand out of range (-1 is not between
  0 and 3)
while with folding enabled, and this modulo in place, we compile without
warning.  So there is a behavior change, for which I have mixed
feelings.  :-)


> > +	tree splat;
> > +	if (TREE_CODE (arg0) == VECTOR_CST)
> > +	  splat = VECTOR_CST_ELT (arg0, TREE_INT_CST_LOW (new_arg1));
> > +	else
> > +	  {
> > +	    /* Determine (in bits) the length and start location of the
> > +	       splat value for a call to the tree_vec_extract helper.  */
> > +	    int tree_size_in_bits = TREE_INT_CST_LOW (size_in_bytes (arg0_type))
> > +				    * BITS_PER_UNIT;
> > +	    int splat_elem_size = tree_size_in_bits / n_elts;
> > +	    int splat_start_bit = TREE_INT_CST_LOW (new_arg1) * splat_elem_size;
> > +	    /* Do not attempt to early-fold if the size + specified offset into
> > +	       the vector would touch outside of the source vector.  */
> > +	    tree len = build_int_cst (bitsizetype, splat_elem_size);
> > +	    tree start = build_int_cst (bitsizetype, splat_start_bit);
> > +	    splat = tree_vec_extract (gsi, TREE_TYPE (lhs_type), arg0,
> > +				      len, start);
> > +	}
> 
> This closing brace should be indented two spaces more.
Ok

> > -static inline tree
> > +tree
> >  tree_vec_extract (gimple_stmt_iterator *gsi, tree type,
> >  		  tree t, tree bitsize, tree bitpos)
> 
> It could use a comment, too (what the args are, etc.)

I can definitely add some commentary to my call into this function.
At a glance, the functions nearby tree_vec_extract do have a couple
lines of description each, so I'll look and see if I can come up with
anything reasonable here.

> Other than those nits, looks fine to me.  Maybe Richard or Bill have
> more comments?

Thanks for the review. 
-Will

> 
> 
> Segher
>