| 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 | expand |
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
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
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
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
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
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
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.
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);
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(-)