Re: [PATCH, rs6000] Fix PR81622

[Bill Schmidt]

> On Jul 31, 2017, at 11:27 AM, Jakub Jelinek  wrote:
> 
> On Mon, Jul 31, 2017 at 11:19:26AM -0500, Bill Schmidt wrote:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81622 reports an ICE-on-invalid
>> for the vec_ld and vec_st built-in functions.  This fires when the last
>> argument of the built-in is not a pointer or array type, as is required.
>> We break on this during early expansion of the built-ins into tree code
>> during parsing.  The solution, as with other ill-formed uses of these
>> built-ins, is to avoid the early expansion when the argument has an invalid
>> type, so that normal error handling can kick in later.
>> 
>> (The long-term solution is to move the vec_ld and vec_st built-ins to the
>> gimple folding work that Will Schmidt has been doing, but that hasn't
>> happened yet.)
>> 
>> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
>> Is this ok for trunk and GCC 7?  I'd like to get it into 7.2 since it
>> is a 7 regression.
> 
> See the patch I've attached in the PR, this isn't sufficient
> (and for the ARRAY_TYPE I wonder if you can ever see there ARRAY_TYPE),
> the function has various other issues, including e.g. ICE on
> vec_cmpne (1, 2) with -mpower9.

Yes, I'll withdraw the patch (but the ARRAY_TYPE thing is necessary as
we've discussed).  I'll step out of your way on this one since you've got it
well in hand.  It would be great to have a fix in for 7.2, though.

Bill

> 
>   Jakub
> 

Re: [PATCH, rs6000] Fix PR81622

[Jakub Jelinek]

On Mon, Jul 31, 2017 at 11:19:26AM -0500, Bill Schmidt wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81622 reports an ICE-on-invalid
> for the vec_ld and vec_st built-in functions.  This fires when the last
> argument of the built-in is not a pointer or array type, as is required.
> We break on this during early expansion of the built-ins into tree code
> during parsing.  The solution, as with other ill-formed uses of these
> built-ins, is to avoid the early expansion when the argument has an invalid
> type, so that normal error handling can kick in later.
> 
> (The long-term solution is to move the vec_ld and vec_st built-ins to the
> gimple folding work that Will Schmidt has been doing, but that hasn't
> happened yet.)
> 
> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
> Is this ok for trunk and GCC 7?  I'd like to get it into 7.2 since it
> is a 7 regression.

See the patch I've attached in the PR, this isn't sufficient
(and for the ARRAY_TYPE I wonder if you can ever see there ARRAY_TYPE),
the function has various other issues, including e.g. ICE on
vec_cmpne (1, 2) with -mpower9.

Jakub

[PATCH, rs6000] Fix PR81622

[Bill Schmidt]

Hi,

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81622 reports an ICE-on-invalid
for the vec_ld and vec_st built-in functions.  This fires when the last
argument of the built-in is not a pointer or array type, as is required.
We break on this during early expansion of the built-ins into tree code
during parsing.  The solution, as with other ill-formed uses of these
built-ins, is to avoid the early expansion when the argument has an invalid
type, so that normal error handling can kick in later.

(The long-term solution is to move the vec_ld and vec_st built-ins to the
gimple folding work that Will Schmidt has been doing, but that hasn't
happened yet.)

Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
Is this ok for trunk and GCC 7?  I'd like to get it into 7.2 since it
is a 7 regression.

Thanks,
Bill


[gcc]

2017-07-31  Bill Schmidt  

PR target/81622
* config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin):
Don't expand vec_ld or vec_st early if the last argument isn't a
pointer or array type.

[gcc/testsuite]

2017-07-31  Bill Schmidt  

PR target/81622
* gcc.target/powerpc/pr81622.c: New file.


Index: gcc/config/rs6000/rs6000-c.c
===
--- gcc/config/rs6000/rs6000-c.c(revision 250721)
+++ gcc/config/rs6000/rs6000-c.c(working copy)
@@ -6454,10 +6454,13 @@ altivec_resolve_overloaded_builtin (location_t loc
  consider this for all memory access built-ins.
 
  When -maltivec=be is specified, or the wrong number of arguments
- is provided, simply punt to existing built-in processing.  */
+ is provided, or the second argument isn't a pointer, simply punt
+ to existing built-in processing.  */
   if (fcode == ALTIVEC_BUILTIN_VEC_LD
   && (BYTES_BIG_ENDIAN || !VECTOR_ELT_ORDER_BIG)
-  && nargs == 2)
+  && nargs == 2
+  && (POINTER_TYPE_P ((*arglist)[1])
+ || TREE_CODE (TREE_TYPE ((*arglist)[1])) == ARRAY_TYPE))
 {
   tree arg0 = (*arglist)[0];
   tree arg1 = (*arglist)[1];
@@ -6528,7 +6531,9 @@ altivec_resolve_overloaded_builtin (location_t loc
   /* Similarly for stvx.  */
   if (fcode == ALTIVEC_BUILTIN_VEC_ST
   && (BYTES_BIG_ENDIAN || !VECTOR_ELT_ORDER_BIG)
-  && nargs == 3)
+  && nargs == 3
+  && (POINTER_TYPE_P ((*arglist)[2])
+ || TREE_CODE (TREE_TYPE ((*arglist)[2])) == ARRAY_TYPE))
 {
   tree arg0 = (*arglist)[0];
   tree arg1 = (*arglist)[1];
Index: gcc/testsuite/gcc.target/powerpc/pr81622.c
===
--- gcc/testsuite/gcc.target/powerpc/pr81622.c  (nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr81622.c  (working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+
+void a()
+{
+  __builtin_vec_ld (1, 2); /* { dg-error "invalid parameter combination for 
AltiVec intrinsic __builtin_vec_ld" } */
+}
+
+void b()
+{
+  __builtin_vec_st (0, 1, 2); /* { dg-error "invalid parameter combination for 
AltiVec intrinsic __builtin_vec_st" } */
+}