| 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);
Hi, 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. The fix for the root problem is just to add some missing cases to existing error checking. To help reduce the mystery of the follow-up ICE, I added some logic to the vector move pattern in vector.md to check for a scalar constant being used in a vector context, and forced an ICE with an explanatory message at that point. The results look like this: wschmidt@pike:~/src$ $GCC_INSTALL/bin/gcc pr80376.c pr80376.c: In function 'main': pr80376.c:12:5: error: argument 3 must be a 2-bit unsigned literal vec_vsx_st(vec_xxpermdi(a, b, j), 0, c); ^~~~~~~~~~ pr80376.c:12:5: internal compiler error: non-vector constant found where vector expected 0x116cde0b gen_movv4si(rtx_def*, rtx_def*) /home/wschmidt/gcc/gcc-mainline-test/gcc/config/rs6000/vector.md:114 0x105fd113 insn_gen_fn::operator()(rtx_def*, rtx_def*) const /home/wschmidt/gcc/gcc-mainline-test/gcc/recog.h:301 0x1077a497 emit_move_insn_1(rtx_def*, rtx_def*) /home/wschmidt/gcc/gcc-mainline-test/gcc/expr.c:3643 0x1077ab37 emit_move_insn(rtx_def*, rtx_def*) /home/wschmidt/gcc/gcc-mainline-test/gcc/expr.c:3738 0x107826d3 store_expr_with_bounds(tree_node*, rtx_def*, int, bool, bool, tree_node*) /home/wschmidt/gcc/gcc-mainline-test/gcc/expr.c:5729 0x107802a3 expand_assignment(tree_node*, tree_node*, bool) /home/wschmidt/gcc/gcc-mainline-test/gcc/expr.c:5321 0x1056ec0b expand_call_stmt /home/wschmidt/gcc/gcc-mainline-test/gcc/cfgexpand.c:2656 0x10572b87 expand_gimple_stmt_1 /home/wschmidt/gcc/gcc-mainline-test/gcc/cfgexpand.c:3571 0x105734c3 expand_gimple_stmt /home/wschmidt/gcc/gcc-mainline-test/gcc/cfgexpand.c:3737 0x1057cd03 expand_gimple_basic_block /home/wschmidt/gcc/gcc-mainline-test/gcc/cfgexpand.c:5744 0x1057ef3b execute /home/wschmidt/gcc/gcc-mainline-test/gcc/cfgexpand.c:6357 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <https://gcc.gnu.org/bugs/> for instructions. I think this is the best way to handle this for stage 4. At some point we should review our error handling and see whether we can produce better replacement values than const0_rtx, that won't immediately ICE when used in a call argument context. This seems like too much churn for late stage 4. I've also fixed the documentation to clarify that the third argument to vec_xxpermdi must be a constant. Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions. Is this ok for trunk, and eventual backport to GCC 6? Thanks, Bill 2017-04-10 Bill Schmidt <wschmidt@linux.vnet.ibm.com> PR target/80376 * config/rs6000/rs6000.c (rs6000_expand_ternop_builtin): Add missing vsx_xxpermdi_* variants. * config/rs6000/vector.md (mov<mode>): Add pre-emptive ICE when a non-vector constant is used in a vector context. * doc/extend.texi: Document that vec_xxpermdi's third argument must be a constant.