On Fri, 2018-04-13 at 17:53 -0500, Segher Boessenkool wrote:
> Hi!
>
> On Fri, Apr 13, 2018 at 03:27:40PM -0700, Carl Love wrote:
> > On Fri, 2018-04-13 at 16:54 -0500, Segher Boessenkool wrote:
> > > On Fri, Apr 13, 2018 at 09:49:25AM -0700, Carl Love wrote:
> > > > diff --git a/gcc/config/rs6000/rs6000.c
> > > > b/gcc/config/rs6000/rs6000.c
> > > > index a0c9b5c..855be43 100644
> > > > --- a/gcc/config/rs6000/rs6000.c
> > > > +++ b/gcc/config/rs6000/rs6000.c
> > > > @@ -16576,8 +16576,9 @@ rs6000_gimple_fold_builtin
> > > > (gimple_stmt_iterator *gsi)
> > > > {
> > > > arg0 = gimple_call_arg (stmt, 0);
> > > > lhs = gimple_call_lhs (stmt);
> > > > - /* Only fold the vec_splat_*() if arg0 is
> > > > constant. */
> > > > - if (TREE_CODE (arg0) != INTEGER_CST)
> > > > + /* Only fold the vec_splat_*() if arg0 is a 5-bit
> > > > constant. */
> > > > + if (TREE_CODE (arg0) != INTEGER_CST
> > > > + || TREE_INT_CST_LOW (arg0) & ~0x1f)
> > > > return false;
> > >
> > > Should this test for *signed* 5-bit constants only?
> > >
> > > if (TREE_CODE (arg0) != INTEGER_CST
> > > || !IN_RANGE (TREE_INT_CST_LOW (arg0), -16, 15))
> >
> > The buitins for the unsigned vec_splat_u[8, 16,32] are actually
> > mapped
> > to their corresponding signed version vec_splat_s[8, 16,32] in
> > altivec.h. Both the vec_splat_u[8, 16,32] and vec_splat_s[8,
> > 16,32]
> > builtins will get you to the case statement
> >
> > /* flavors of vec_splat_[us]{8,16,32}. */
> > case ALTIVEC_BUILTIN_VSPLTISB:
> > case ALTIVEC_BUILTIN_VSPLTISH:
> > case ALTIVEC_BUILTIN_VSPLTISW:
> >
> > under which the change is being made. So technically arg0 could be
> > a
> > signed 5-bit or an unsiged 5-bit value. Either way the 5-bit value
> > is
> > splatted into the vector with sign extension to 8/16/32 bits. So I
> > would argue that the IN_RANGE test for signed values is maybe
> > misleading as arg0 could represent a signed or unsigned
> > value. Both
> > tests, the one from the patch or Segher's suggestion, are really
> > just
> > looking to see if any of the upper bits are 1. From a functional
> > standpoint, I don't see any difference and feel either one would do
> > the
> > job. Am I missing anything?
>
> But then the & ~0x1f test is not good either, it does not work for
> values
> -16..-1 ?
>
> You cannot handle signed and unsigned exactly the same for the test
> for
> allowed values.
>
Segher:
The VSPLTIS[S|H|W] can only be used if the value in arg0 is a bit
pattern consisting of at most 5-bits. The issue is the field in the
vector splat instruction is only five bits wide. So the value of
TREE_INT_CST_LOW (arg0) & ~0x1f) will be non-zero if the value requires
more then 5-bits to represent. The if statement will thus evaluate to
TRUE and thus the code returns false indicating we can not do the
folding since the constant requires more then a 5-bit field to
represent. I don't think we really care how the 5-bits are interpreted
as a signed or unsigned value just that the value can be represented by
at most 5-bits.
The test
if (TREE_CODE (arg0) != INTEGER_CST
|| TREE_INT_CST_LOW (arg0) & ~0x1f)
return false;
is really checking if the value is not an integer or the value is not a
5-bit constant. It seems to me this should work fine for signed and
unsigned 5-bit numbers.
Carl
