Message ID | 1550569136-751-1-git-send-email-helijia@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | rs6000: Fix PR 88100, range check for vec_splat_{su}{8,16,32} | expand |
Hi! On Tue, Feb 19, 2019 at 03:38:56AM -0600, Li Jia He wrote: > GCC revision 259524 implemented range check for the vec_splat_{su}{8,16,32} > builtins. However, as a consequence of the implementation, the range check > is not done correctly for the expected vspltis[bhw] instructions. The result > is that we may not get a valid error message if the valid range of the data > is exceeded. > PR target/88100 > * gcc/config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Remove > sext_hwi in IN_RANGE (sext_hwi (TREE_INT_CST_LOW (arg0), size), > -16, 15). Please avoid more than a word or two of C in changelogs. Also, in such cases you should show the case labels: PR target/88100 * gcc/config/rs6000/rs6000.c (rs6000_gimple_fold_builtin) <case ALTIVEC_BUILTIN_VSPLTISB, ALTIVEC_BUILTIN_VSPLTISH, ALTIVEC_BUILTIN_VSPLTISW>: Don't convert the operand before range checking it. Okay for trunk with that. Thanks! Segher
On 2/19/19 8:11 AM, Segher Boessenkool wrote: > Hi! > > On Tue, Feb 19, 2019 at 03:38:56AM -0600, Li Jia He wrote: >> GCC revision 259524 implemented range check for the vec_splat_{su}{8,16,32} >> builtins. However, as a consequence of the implementation, the range check >> is not done correctly for the expected vspltis[bhw] instructions. The result >> is that we may not get a valid error message if the valid range of the data >> is exceeded. >> PR target/88100 >> * gcc/config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Remove >> sext_hwi in IN_RANGE (sext_hwi (TREE_INT_CST_LOW (arg0), size), >> -16, 15). > Please avoid more than a word or two of C in changelogs. Also, in such > cases you should show the case labels: > > PR target/88100 > * gcc/config/rs6000/rs6000.c (rs6000_gimple_fold_builtin) > <case ALTIVEC_BUILTIN_VSPLTISB, ALTIVEC_BUILTIN_VSPLTISH, > ALTIVEC_BUILTIN_VSPLTISW>: Don't convert the operand before range > checking it. > > Okay for trunk with that. Thanks! I believe you will also want to get permission to backport this to GCC 8. It looks like the problem was introduced there, correct? Thanks, Bill > > > Segher >
Yes, you are correct. If there is no question with this patch, I will backport this to GCC 8. Thanks, Lijia On 2019/2/20 12:13 PM, Bill Schmidt wrote: > On 2/19/19 8:11 AM, Segher Boessenkool wrote: >> Hi! >> >> On Tue, Feb 19, 2019 at 03:38:56AM -0600, Li Jia He wrote: >>> GCC revision 259524 implemented range check for the vec_splat_{su}{8,16,32} >>> builtins. However, as a consequence of the implementation, the range check >>> is not done correctly for the expected vspltis[bhw] instructions. The result >>> is that we may not get a valid error message if the valid range of the data >>> is exceeded. >>> PR target/88100 >>> * gcc/config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Remove >>> sext_hwi in IN_RANGE (sext_hwi (TREE_INT_CST_LOW (arg0), size), >>> -16, 15). >> Please avoid more than a word or two of C in changelogs. Also, in such >> cases you should show the case labels: >> >> PR target/88100 >> * gcc/config/rs6000/rs6000.c (rs6000_gimple_fold_builtin) >> <case ALTIVEC_BUILTIN_VSPLTISB, ALTIVEC_BUILTIN_VSPLTISH, >> ALTIVEC_BUILTIN_VSPLTISW>: Don't convert the operand before range >> checking it. >> >> Okay for trunk with that. Thanks! > > I believe you will also want to get permission to backport this to GCC 8. > It looks like the problem was introduced there, correct? > > Thanks, > Bill > >> >> Segher >> >
[ Don't top-post please ] On Wed, Feb 20, 2019 at 12:35:37PM +0800, Li Jia He wrote: > >I believe you will also want to get permission to backport this to GCC 8. > >It looks like the problem was introduced there, correct? > Yes, you are correct. If there is no question with this patch, > I will backport this to GCC 8. It is okay for backport to 8. Segher
On Wed, Feb 20, 2019 at 08:34:41AM -0600, Segher Boessenkool wrote: > [ Don't top-post please ] > > On Wed, Feb 20, 2019 at 12:35:37PM +0800, Li Jia He wrote: > > >I believe you will also want to get permission to backport this to GCC 8. > > >It looks like the problem was introduced there, correct? > > > Yes, you are correct. If there is no question with this patch, > > I will backport this to GCC 8. > > It is okay for backport to 8. Can that be deferred for 8.4? We are very close to release and I'd prefer not to do another release candidate. Jakub
On 2/20/19 8:36 AM, Jakub Jelinek wrote: > On Wed, Feb 20, 2019 at 08:34:41AM -0600, Segher Boessenkool wrote: >> [ Don't top-post please ] >> >> On Wed, Feb 20, 2019 at 12:35:37PM +0800, Li Jia He wrote: >>>> I believe you will also want to get permission to backport this to GCC 8. >>>> It looks like the problem was introduced there, correct? >>> Yes, you are correct. If there is no question with this patch, >>> I will backport this to GCC 8. >> It is okay for backport to 8. > Can that be deferred for 8.4? We are very close to release and I'd prefer > not to do another release candidate. Definitely, this is not urgent. Bill > > Jakub >
On Wed, Feb 20, 2019 at 03:36:18PM +0100, Jakub Jelinek wrote: > On Wed, Feb 20, 2019 at 08:34:41AM -0600, Segher Boessenkool wrote: > > [ Don't top-post please ] > > > > On Wed, Feb 20, 2019 at 12:35:37PM +0800, Li Jia He wrote: > > > >I believe you will also want to get permission to backport this to GCC 8. > > > >It looks like the problem was introduced there, correct? > > > > > Yes, you are correct. If there is no question with this patch, > > > I will backport this to GCC 8. > > > > It is okay for backport to 8. > > Can that be deferred for 8.4? We are very close to release and I'd prefer > not to do another release candidate. Yes, sorry... After 8.3 is out it is okay for 8, I meant. Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index aea7925..b1249bc 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -16105,22 +16105,13 @@ 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 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)) + || !IN_RANGE (TREE_INT_CST_LOW (arg0), -16, 15)) return false; gimple_seq stmts = NULL; location_t loc = gimple_location (stmt); diff --git a/gcc/testsuite/gcc.target/powerpc/pr88100.c b/gcc/testsuite/gcc.target/powerpc/pr88100.c new file mode 100644 index 0000000..4452145 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr88100.c @@ -0,0 +1,44 @@ +/* PR88100. Verify that rs6000 gimple-folding code handles the + vec_splat_{su}{8,16,32} invalid data properly. */ + +/* { dg-do compile } */ +/* { dg-require-effective-target powerpc_altivec_ok } */ +/* { dg-options "-maltivec" } */ + +#include <altivec.h> + +vector unsigned char +splatu1 (void) +{ + return vec_splat_u8(0x100);/* { dg-error "argument 1 must be a 5-bit signed literal" } */ +} + +vector unsigned short +splatu2 (void) +{ + return vec_splat_u16(0x10000);/* { dg-error "argument 1 must be a 5-bit signed literal" } */ +} + +vector unsigned int +splatu3 (void) +{ + return vec_splat_u32(0x10000000);/* { dg-error "argument 1 must be a 5-bit signed literal" } */ +} + +vector signed char +splats1 (void) +{ + return vec_splat_s8(0x100);/* { dg-error "argument 1 must be a 5-bit signed literal" } */ +} + +vector signed short +splats2 (void) +{ + return vec_splat_s16(0x10000);/* { dg-error "argument 1 must be a 5-bit signed literal" } */ +} + +vector signed int +splats3 (void) +{ + return vec_splat_s32(0x10000000);/* { dg-error "argument 1 must be a 5-bit signed literal" } */ +}