PR 83402 Fix ICE for vec_splat_s8, vec_splat_s16, vec_splat_s32 builtins

Message ID 1523638165.12016.22.camel@us.ibm.com
State New
Headers show
Series
  • PR 83402 Fix ICE for vec_splat_s8, vec_splat_s16, vec_splat_s32 builtins
Related show

Commit Message

Carl Love April 13, 2018, 4:49 p.m.
GCC Maintainers:

GCC revision 255549  implemented early gimple folding for the
vec_splat_s[8,16,32] builtins.  However, as a consequence of the
implementation, we lost error checking for out-of-range values for the
expected vspltis[bhw] instructions.  The result of not having the out-
of-range checking is we get and ICE.

This patch adds the missing error checking on argument zero for the
vec_splat_s[8,16,32] builtins.  The argument must be a 5-bit const int
as specified for the vspltis[bhw] instructions.

The regression testing for the patch was done on GCC mainline on 

    powerpc64le-unknown-linux-gnu (Power 8 LE)

with no regressions.  Additional hand testing was done as well to test
the various cases such as

vec_splat_s8 (31)
vec_splat_s8 (32)
vec_splat_s8 (value) where "value" is an integer variable

to verify vector result is correct for a 5-bit const int argument (
i.e. 31).  The error message "error: argument 1 must be a 5-bit signed
literal" is generated in the other cases.

Please let me know if the patch looks OK for the GCC 7 branch.

                         Carl Love
---------------------------------------------------


gcc/ChangeLog:

2018-04-13  Carl Love  <cel@us.ibm.com>

        PR target/83402
	* config/rs6000/rs6000-c.c (rs6000_gimple_fold_builtin): Add
	size check for arg0.
---
 gcc/config/rs6000/rs6000.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Segher Boessenkool April 13, 2018, 9:54 p.m. | #1
Hi Carl,

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

or similar?

Approved for trunk (with such a change if appropriate, and testing
then of course).  Thanks!


Segher
Carl Love April 13, 2018, 10:27 p.m. | #2
On Fri, 2018-04-13 at 16:54 -0500, Segher Boessenkool wrote:
> Hi Carl,
> 
> 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))
Segher:

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?

                       Carl Love
Segher Boessenkool April 13, 2018, 10:53 p.m. | #3
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
Carl Love April 16, 2018, 4:26 p.m. | #4
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
Segher Boessenkool April 16, 2018, 9:31 p.m. | #5
On Mon, Apr 16, 2018 at 09:26:13AM -0700, Carl Love wrote:
> > 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.

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

All negative numbers (-16..-1 here) have bits outside 0x1f set (all bits
outside it).


Segher
Carl Love April 18, 2018, 5:37 p.m. | #6
GCC Maintainers:

I have addressed Segher's concerns regarding the range checking of the
argument.  The following is the updated patch.  

The regression testing for the patch was done on GCC mainline on 

    powerpc64le-unknown-linux-gnu (Power 8 LE)

with no regressions.  Additional hand testing was done on the following
test cases to verify the range checking was working.

vec_splat_u8 (0xfffffffe);
vec_splat_u8 (0x7e);
vec_splat_u8 (0xfe);
vec_splat_u16 (0xfffffffe);
vec_splat_u16 (0x7ffe);
vec_splat_u16 (0xfffe);
vec_splat_u16 (-2);
vec_splat_u32 (0xfffffffe);
vec_splat_u32 (0xeffffffe);
vec_splat_s32 (16);
vec_splat_s32 (15);
vec_splat_s32 (-16);
vec_splat_s32 (-17);
vec_splat_s8 (16);
vec_splat_s8 (15);
vec_splat_s8 (1);
vec_splat_s8 (-1);
vec_splat_s8 (-16);
vec_splat_s8 (-17);

Please let me know if the patch looks OK for the GCC 7 branch.

                         Carl Love
--------------------------------------------------

gcc/ChangeLog:

2018-04-17  Carl Love  <cel@us.ibm.com>

        PR target/83402
        * config/rs6000/rs6000-c.c (rs6000_gimple_fold_builtin): Add
        size check for arg0.
---
 gcc/config/rs6000/rs6000.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index a0c9b5c..6b8039d 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -16574,10 +16574,23 @@ rs6000_gimple_fold_builtin
(gimple_stmt_iterator *gsi)
     case ALTIVEC_BUILTIN_VSPLTISH:
     case ALTIVEC_BUILTIN_VSPLTISW:
       {
+	 int size;
+
+         if (fn_code == ALTIVEC_BUILTIN_VSPLTISB)
+           size = 8;
+         else if (fn_code == ALTIVEC_BUILTIN_VSPLTISH)
+           size = 16;
+         else
+           size = 32;
+
 	 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 the lower bits of arg 0 is
a
+	    5-bit signed constant in range -16 to +15.  */
+	 if (TREE_CODE (arg0) != INTEGER_CST
+	     || !IN_RANGE (sext_hwi(TREE_INT_CST_LOW (arg0), size),
+			   -16, 15))
 	   return false;
 	 gimple_seq stmts = NULL;
 	 location_t loc = gimple_location (stmt);
-- 
2.7.4
Segher Boessenkool April 19, 2018, 9:55 p.m. | #7
On Wed, Apr 18, 2018 at 10:37:41AM -0700, Carl Love wrote:
> Please let me know if the patch looks OK for the GCC 7 branch.

I think it looks fine.  But it should go to trunk, first?

Okay for trunk, and for the branches after a suitable delay.  Thanks!


Segher


> 2018-04-17  Carl Love  <cel@us.ibm.com>
> 
>         PR target/83402
>         * config/rs6000/rs6000-c.c (rs6000_gimple_fold_builtin): Add
>         size check for arg0.

Patch

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;
 	 gimple_seq stmts = NULL;
 	 location_t loc = gimple_location (stmt);