This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH, rs6000] Fix PR80376 (somewhat)


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.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]