diff mbox series

rs6000: Fix PR 88100, range check for vec_splat_{su}{8,16,32}

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

Commit Message

Li Jia He Feb. 19, 2019, 9:38 a.m. UTC 
Hi,

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.

Although the input of the function prototype of vec_splat_{su}{8,16,32} is
const int, the actual data usage range is limited to the data range of 5 bits
signed.  We should limit the int_cst.val[0] data to the 5 bit signed data range
without any modification in the input arg0 parameter.  However, the sext_hwi
function intercepts the data of TREE_INT_CST_LOW (arg0) as size bits in the
sext_hwi (TREE_INT_CST_LOW (arg0), size) statement.  This will cause some of
the excess data to fall within the range of 5 bits signed, so that the correct
diagnostic information cannot be generated, we need to remove the sext_hwi to
ensure that the input data has not been modified.

This patch fix range check 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.  Is it OK for trunk?

Thanks,
Lijia

---
gcc/ChangeLog

2019-02-15  Li Jia He  <helijia@linux.ibm.com>

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

gcc/testsuite/ChangeLog

2019-02-15  Li Jia He  <helijia@linux.ibm.com>

	PR target/88100
	* gcc/testsuite/gcc.target/powerpc/pr88100.c: New testcase.
---
 gcc/config/rs6000/rs6000.c                 | 11 +-------
 gcc/testsuite/gcc.target/powerpc/pr88100.c | 44 ++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr88100.c

Comments

Segher Boessenkool Feb. 19, 2019, 2:11 p.m. UTC | #1 
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
Bill Schmidt Feb. 20, 2019, 4:13 a.m. UTC | #2 
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
>
Li Jia He Feb. 20, 2019, 4:35 a.m. UTC | #3 
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
>>
>
Segher Boessenkool Feb. 20, 2019, 2:34 p.m. UTC | #4 
[ 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
Jakub Jelinek Feb. 20, 2019, 2:36 p.m. UTC | #5 
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
Bill Schmidt Feb. 20, 2019, 2:40 p.m. UTC | #6 
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
>
Segher Boessenkool Feb. 20, 2019, 4:21 p.m. UTC | #7 
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 mbox series

Patch

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" } */
+}