> On Aug 1, 2017, at 1:40 AM, Jakub Jelinek <ja...@redhat.com> wrote: > > Hi! > > On Mon, Jul 31, 2017 at 02:42:21PM -0500, Bill Schmidt wrote: >>> On Jul 31, 2017, at 11:27 AM, Jakub Jelinek <ja...@redhat.com> 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. > > Here is the variant patch. In addition to fixing the ICE for vec_ld, for > vec_st it just moves the premature computation of aligned to the point where > it is used and that is after we've also verified that the types of the call > arguments match the builtin argument types, it fixes ICE on vec_cmpne, where > for -mpower9 it accesses TYPE_MODE (TREE_TYPE (arg0_type)) without checking > that arg0_type is actually VECTOR_TYPE (if it is e.g. INTEGRAL_TYPE, it will > segfault) and has some formatting fixes too. > > Bootstrapped/regtested on powerpc64{,le}-linux, ok for > trunk/7.2 (for the latter it applies without the 2 formatting fix hunks > with while (desc->code && desc->code == fcode &&)? > > Note, there is diagnostic message in that routine starting with "Builtin, > that is something that should be fixed too, GCC diagnostic messages > (except for Fortran) don't start with a capital letter. But this change > requires to adjust quite a lot of testcases in gcc.target/powerpc :(.
Jakub, thanks for handling! I'll put the cleanup of this diagnostic message on our list. Thanks for pointing it out. Bill > > 2017-07-31 Jakub Jelinek <ja...@redhat.com> > > PR target/81622 > * config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin): For > __builtin_vec_cmpne verify both arguments are compatible vectors > before looking at TYPE_MODE on the element type. For __builtin_vec_ld > verify arg1_type is a pointer or array type. For __builtin_vec_st, > move computation of aligned to after checking the argument types. > Formatting fixes. > > * gcc.target/powerpc/pr81622.c: New test. > > --- gcc/config/rs6000/rs6000-c.c.jj 2017-07-25 12:19:51.000000000 +0200 > +++ gcc/config/rs6000/rs6000-c.c 2017-07-31 17:05:59.947783972 +0200 > @@ -5852,6 +5852,12 @@ altivec_resolve_overloaded_builtin (loca > tree arg1 = (*arglist)[1]; > tree arg1_type = TREE_TYPE (arg1); > > + /* Both arguments must be vectors and the types must be compatible. */ > + if (TREE_CODE (arg0_type) != VECTOR_TYPE) > + goto bad; > + if (!lang_hooks.types_compatible_p (arg0_type, arg1_type)) > + goto bad; > + > /* Power9 instructions provide the most efficient implementation of > ALTIVEC_BUILTIN_VEC_CMPNE if the mode is not DImode or TImode > or SFmode or DFmode. */ > @@ -5861,12 +5867,6 @@ altivec_resolve_overloaded_builtin (loca > || (TYPE_MODE (TREE_TYPE (arg0_type)) == SFmode) > || (TYPE_MODE (TREE_TYPE (arg0_type)) == DFmode)) > { > - /* Both arguments must be vectors and the types must be compatible. > */ > - if (TREE_CODE (arg0_type) != VECTOR_TYPE) > - goto bad; > - if (!lang_hooks.types_compatible_p (arg0_type, arg1_type)) > - goto bad; > - > switch (TYPE_MODE (TREE_TYPE (arg0_type))) > { > /* vec_cmpneq (va, vb) == vec_nor (vec_cmpeq (va, vb), > @@ -5931,8 +5931,8 @@ altivec_resolve_overloaded_builtin (loca > __int128) and the types must be compatible. */ > if (TREE_CODE (arg0_type) != VECTOR_TYPE) > goto bad; > - if (!lang_hooks.types_compatible_p (arg0_type, arg1_type) || > - !lang_hooks.types_compatible_p (arg1_type, arg2_type)) > + if (!lang_hooks.types_compatible_p (arg0_type, arg1_type) > + || !lang_hooks.types_compatible_p (arg1_type, arg2_type)) > goto bad; > > switch (TYPE_MODE (TREE_TYPE (arg0_type))) > @@ -6014,8 +6014,8 @@ altivec_resolve_overloaded_builtin (loca > __int128) and the types must be compatible. */ > if (TREE_CODE (arg0_type) != VECTOR_TYPE) > goto bad; > - if (!lang_hooks.types_compatible_p (arg0_type, arg1_type) || > - !lang_hooks.types_compatible_p (arg1_type, arg2_type)) > + if (!lang_hooks.types_compatible_p (arg0_type, arg1_type) > + || !lang_hooks.types_compatible_p (arg1_type, arg2_type)) > goto bad; > > switch (TYPE_MODE (TREE_TYPE (arg0_type))) > @@ -6464,6 +6464,9 @@ altivec_resolve_overloaded_builtin (loca > > /* Strip qualifiers like "const" from the pointer arg. */ > tree arg1_type = TREE_TYPE (arg1); > + if (!POINTER_TYPE_P (arg1_type) && TREE_CODE (arg1_type) != ARRAY_TYPE) > + goto bad; > + > tree inner_type = TREE_TYPE (arg1_type); > if (TYPE_QUALS (TREE_TYPE (arg1_type)) != 0) > { > @@ -6552,11 +6555,6 @@ altivec_resolve_overloaded_builtin (loca > arg2 = build1 (ADDR_EXPR, arg2_type, arg2_elt0); > } > > - tree addr = fold_build2_loc (loc, POINTER_PLUS_EXPR, arg2_type, > - arg2, arg1); > - tree aligned = fold_build2_loc (loc, BIT_AND_EXPR, arg2_type, addr, > - build_int_cst (arg2_type, -16)); > - > /* Find the built-in to make sure a compatible one exists; if not > we fall back to default handling to get the error message. */ > for (desc = altivec_overloaded_builtins; > @@ -6569,6 +6567,12 @@ altivec_resolve_overloaded_builtin (loca > && rs6000_builtin_type_compatible (TREE_TYPE (arg2), > desc->op3)) > { > + tree addr = fold_build2_loc (loc, POINTER_PLUS_EXPR, arg2_type, > + arg2, arg1); > + tree aligned > + = fold_build2_loc (loc, BIT_AND_EXPR, arg2_type, > + addr, build_int_cst (arg2_type, -16)); > + > tree arg0_type = TREE_TYPE (arg0); > if (TYPE_MODE (arg0_type) == V2DImode) > /* Type-based aliasing analysis thinks vector long > @@ -6694,8 +6698,8 @@ altivec_resolve_overloaded_builtin (loca > overloaded_code = P6_BUILTIN_CMPB_32; > } > > - while (desc->code && desc->code == fcode && > - desc->overloaded_code != overloaded_code) > + while (desc->code && desc->code == fcode > + && desc->overloaded_code != overloaded_code) > desc++; > > if (desc->code && (desc->code == fcode) > @@ -6741,8 +6745,8 @@ altivec_resolve_overloaded_builtin (loca > else > overloaded_code = P9V_BUILTIN_VSIEDP; > } > - while (desc->code && desc->code == fcode && > - desc->overloaded_code != overloaded_code) > + while (desc->code && desc->code == fcode > + && desc->overloaded_code != overloaded_code) > desc++; > if (desc->code && (desc->code == fcode) > && rs6000_builtin_type_compatible (types[0], desc->op1) > @@ -6784,9 +6788,9 @@ altivec_resolve_overloaded_builtin (loca > } > } > bad: > - { > - const char *name = rs6000_overloaded_builtin_name (fcode); > - error ("invalid parameter combination for AltiVec intrinsic %s", name); > - return error_mark_node; > - } > + { > + const char *name = rs6000_overloaded_builtin_name (fcode); > + error ("invalid parameter combination for AltiVec intrinsic %s", name); > + return error_mark_node; > + } > } > --- gcc/testsuite/gcc.target/powerpc/pr81622.c.jj 2017-07-31 > 17:16:26.070493869 +0200 > +++ gcc/testsuite/gcc.target/powerpc/pr81622.c 2017-07-31 > 17:16:18.000000000 +0200 > @@ -0,0 +1,13 @@ > +/* PR target/81622 */ > +/* { dg-do compile { target { powerpc*-*-linux* } } } */ > +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { > "-mcpu=power9" } } */ > +/* { dg-require-effective-target powerpc_p9vector_ok } */ > +/* { dg-options "-mcpu=power9 -O2" } */ > + > +void > +foo (void) > +{ > + __builtin_vec_ld (1, 2); /* { dg-error "invalid parameter combination" } > */ > + __builtin_vec_cmpne (1, 2); /* { dg-error "invalid parameter > combination" } */ > + __builtin_vec_st (1, 0, 5); /* { dg-error "invalid parameter > combination" } */ > +} > > > Jakub >