Message ID | f18c0e20-2b3a-be1b-5772-68c511ceca64@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Mon, Apr 10, 2017 at 12:08:50PM -0500, Bill Schmidt wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80376 shows an ICE on invalid > code when using certain flavors of the vec_xxpermdi intrinsic. The root > cause is that we were not checking the parameter for a legal value, and an > illegal value was being provided by the user (only an integer constant is > permissible for argument 3). > > However, this brings up a slightly larger problem, in that the invalid > vec_xxpermdi call is nested within another call. We have a lot of cases > in rs6000.c where we signal an error message and replace the offending > construct with a const0_rtx. In this case, that const0_rtx was being used > as a vector argument to another call, leading to a follow-up ICE that is > not easy to parse. i386 does this: /* Errors in the source file can cause expand_expr to return const0_rtx where we expect a vector. To avoid crashing, use one of the vector clear instructions. */ static rtx safe_vector_operand (rtx x, machine_mode mode) { if (x == const0_rtx) x = CONST0_RTX (mode); return x; } used in e.g. ix86_expand_binop_builtin as if (VECTOR_MODE_P (mode0)) op0 = safe_vector_operand (op0, mode0); We might want something similar :-) > Index: gcc/config/rs6000/rs6000.c > =================================================================== > --- gcc/config/rs6000/rs6000.c (revision 246804) > +++ gcc/config/rs6000/rs6000.c (working copy) > @@ -15586,6 +15586,11 @@ rs6000_expand_ternop_builtin (enum insn_code icode > || icode == CODE_FOR_vsx_xxpermdi_v2di > || icode == CODE_FOR_vsx_xxpermdi_v2df_be > || icode == CODE_FOR_vsx_xxpermdi_v2di_be > + || icode == CODE_FOR_vsx_xxpermdi_v1ti > + || icode == CODE_FOR_vsx_xxpermdi_v4sf > + || icode == CODE_FOR_vsx_xxpermdi_v4si > + || icode == CODE_FOR_vsx_xxpermdi_v8hi > + || icode == CODE_FOR_vsx_xxpermdi_v16qi > || icode == CODE_FOR_vsx_xxsldwi_v16qi > || icode == CODE_FOR_vsx_xxsldwi_v8hi > || icode == CODE_FOR_vsx_xxsldwi_v4si The existing code is indented with spaces only; please keep the style consistent (fixing it is fine, but then the whole block or function). > --- gcc/config/rs6000/vector.md (revision 246804) > +++ gcc/config/rs6000/vector.md (working copy) > @@ -109,6 +109,11 @@ > { > if (CONSTANT_P (operands[1])) > { > + /* Handle cascading error conditions. */ > + if (VECTOR_MODE_P (<MODE>mode) > + && !VECTOR_MODE_P (GET_MODE (operands[1]))) > + internal_error ("non-vector constant found where vector expected"); > + > if (FLOAT128_VECTOR_P (<MODE>mode)) > { > if (!easy_fp_constant (operands[1], <MODE>mode)) You might not need this with the suggestion above? The patch is okay if you want that for now; it's an improvement over what we have. Segher
Hi Segher, > On Apr 10, 2017, at 4:31 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Mon, Apr 10, 2017 at 12:08:50PM -0500, Bill Schmidt wrote: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80376 shows an ICE on invalid >> code when using certain flavors of the vec_xxpermdi intrinsic. The root >> cause is that we were not checking the parameter for a legal value, and an >> illegal value was being provided by the user (only an integer constant is >> permissible for argument 3). >> >> However, this brings up a slightly larger problem, in that the invalid >> vec_xxpermdi call is nested within another call. We have a lot of cases >> in rs6000.c where we signal an error message and replace the offending >> construct with a const0_rtx. In this case, that const0_rtx was being used >> as a vector argument to another call, leading to a follow-up ICE that is >> not easy to parse. > > i386 does this: > > /* Errors in the source file can cause expand_expr to return const0_rtx > where we expect a vector. To avoid crashing, use one of the vector > clear instructions. */ > static rtx > safe_vector_operand (rtx x, machine_mode mode) > { > if (x == const0_rtx) > x = CONST0_RTX (mode); > return x; > } > > used in e.g. ix86_expand_binop_builtin as > if (VECTOR_MODE_P (mode0)) > op0 = safe_vector_operand (op0, mode0); > We might want something similar :-) That's a nice idea. That would at least keep us from having to fix this in 2-3 dozen places. Let me take a look. Thanks! Bill > >> Index: gcc/config/rs6000/rs6000.c >> =================================================================== >> --- gcc/config/rs6000/rs6000.c (revision 246804) >> +++ gcc/config/rs6000/rs6000.c (working copy) >> @@ -15586,6 +15586,11 @@ rs6000_expand_ternop_builtin (enum insn_code icode >> || icode == CODE_FOR_vsx_xxpermdi_v2di >> || icode == CODE_FOR_vsx_xxpermdi_v2df_be >> || icode == CODE_FOR_vsx_xxpermdi_v2di_be >> + || icode == CODE_FOR_vsx_xxpermdi_v1ti >> + || icode == CODE_FOR_vsx_xxpermdi_v4sf >> + || icode == CODE_FOR_vsx_xxpermdi_v4si >> + || icode == CODE_FOR_vsx_xxpermdi_v8hi >> + || icode == CODE_FOR_vsx_xxpermdi_v16qi >> || icode == CODE_FOR_vsx_xxsldwi_v16qi >> || icode == CODE_FOR_vsx_xxsldwi_v8hi >> || icode == CODE_FOR_vsx_xxsldwi_v4si > > The existing code is indented with spaces only; please keep the style > consistent (fixing it is fine, but then the whole block or function). OK. > >> --- gcc/config/rs6000/vector.md (revision 246804) >> +++ gcc/config/rs6000/vector.md (working copy) >> @@ -109,6 +109,11 @@ >> { >> if (CONSTANT_P (operands[1])) >> { >> + /* Handle cascading error conditions. */ >> + if (VECTOR_MODE_P (<MODE>mode) >> + && !VECTOR_MODE_P (GET_MODE (operands[1]))) >> + internal_error ("non-vector constant found where vector expected"); >> + >> if (FLOAT128_VECTOR_P (<MODE>mode)) >> { >> if (!easy_fp_constant (operands[1], <MODE>mode)) > > You might not need this with the suggestion above? > > The patch is okay if you want that for now; it's an improvement over > what we have. > > > Segher >
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 246804) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -15586,6 +15586,11 @@ rs6000_expand_ternop_builtin (enum insn_code icode || icode == CODE_FOR_vsx_xxpermdi_v2di || icode == CODE_FOR_vsx_xxpermdi_v2df_be || icode == CODE_FOR_vsx_xxpermdi_v2di_be + || icode == CODE_FOR_vsx_xxpermdi_v1ti + || icode == CODE_FOR_vsx_xxpermdi_v4sf + || icode == CODE_FOR_vsx_xxpermdi_v4si + || icode == CODE_FOR_vsx_xxpermdi_v8hi + || icode == CODE_FOR_vsx_xxpermdi_v16qi || icode == CODE_FOR_vsx_xxsldwi_v16qi || icode == CODE_FOR_vsx_xxsldwi_v8hi || icode == CODE_FOR_vsx_xxsldwi_v4si Index: gcc/config/rs6000/vector.md =================================================================== --- gcc/config/rs6000/vector.md (revision 246804) +++ gcc/config/rs6000/vector.md (working copy) @@ -109,6 +109,11 @@ { if (CONSTANT_P (operands[1])) { + /* Handle cascading error conditions. */ + if (VECTOR_MODE_P (<MODE>mode) + && !VECTOR_MODE_P (GET_MODE (operands[1]))) + internal_error ("non-vector constant found where vector expected"); + if (FLOAT128_VECTOR_P (<MODE>mode)) { if (!easy_fp_constant (operands[1], <MODE>mode)) Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 246804) +++ gcc/doc/extend.texi (working copy) @@ -17623,20 +17623,21 @@ void vec_vsx_st (vector bool char, int, vector boo void vec_vsx_st (vector bool char, int, unsigned char *); void vec_vsx_st (vector bool char, int, signed char *); -vector double vec_xxpermdi (vector double, vector double, int); -vector float vec_xxpermdi (vector float, vector float, int); -vector long long vec_xxpermdi (vector long long, vector long long, int); +vector double vec_xxpermdi (vector double, vector double, const int); +vector float vec_xxpermdi (vector float, vector float, const int); +vector long long vec_xxpermdi (vector long long, vector long long, const int); vector unsigned long long vec_xxpermdi (vector unsigned long long, - vector unsigned long long, int); -vector int vec_xxpermdi (vector int, vector int, int); + vector unsigned long long, const int); +vector int vec_xxpermdi (vector int, vector int, const int); vector unsigned int vec_xxpermdi (vector unsigned int, - vector unsigned int, int); -vector short vec_xxpermdi (vector short, vector short, int); + vector unsigned int, const int); +vector short vec_xxpermdi (vector short, vector short, const int); vector unsigned short vec_xxpermdi (vector unsigned short, - vector unsigned short, int); -vector signed char vec_xxpermdi (vector signed char, vector signed char, int); + vector unsigned short, const int); +vector signed char vec_xxpermdi (vector signed char, vector signed char, + const int); vector unsigned char vec_xxpermdi (vector unsigned char, - vector unsigned char, int); + vector unsigned char, const int); vector double vec_xxsldi (vector double, vector double, int); vector float vec_xxsldi (vector float, vector float, int);