diff mbox series

, PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument

Message ID 20170829034147.GA15563@ibm-tiger.the-meissners.org
State New
Headers show
Series , PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument | expand

Commit Message

Michael Meissner Aug. 29, 2017, 3:41 a.m. UTC 
One of the local programmers tried to use the __builtin_unpack_vector_int128
function, but his second argument was not the constant 0 or 1.  The compiler
put the 2nd argument into a register, but there wasn't a valid insn for this,
and raised an insn not found message.  GCC should warn about this illegal
usage.

This patch adds such a warning.  While I was mucking about with this built-in
function, I fixed the constraints to allow the 64-bit integer for unpack result
and pack inputs to be in the traditional Altivec registers as well as the
traditional floating point registers.

I did a bootstrap of the compiler, and there were no regressions on a little
endian power8 system.  I verified that the new test is run.  Can I apply this
patch to the trunk?

Can I apply the part of the patch from rs6000.c to the existing GCC 6/7
branches as well after a shake down period?  The patch to rs6000.md is not
appropriate for GCC 6 (since DImode can't go into Altivec registers).  For GCC
7, it would need to be modified to use the 'wi' constraint instead of 'wa',
since GCC 7 still had support for the -mno-upper-regs-di option to control
access to the traditional Altivec register.

[gcc]
2017-08-28  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/82015
	* config/rs6000/rs6000.c (rs6000_expand_binop_builtin): Insure
	that the second argument of the built-in functions to unpack
	128-bit scalar types to 64-bit values is 0 or 1.  Change to use a
	switch statement instead a lot of if statements.
	* config/rs6000/rs6000.md (unpack<mode>, FMOVE128_VSX iterator):
	Allow 64-bit values to be in Altivec registers as well as
	traditional floating point registers.
	(pack<mode>, FMOVE128_VSX iterator): Likewise.

[gcc/testsuite]
2017-08-28  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/82015
	* gcc.target/powerpc/pr82015.c: New test.

Comments

Segher Boessenkool Aug. 29, 2017, 1:31 p.m. UTC | #1 
Hi!

On Mon, Aug 28, 2017 at 11:41:47PM -0400, Michael Meissner wrote:
> One of the local programmers tried to use the __builtin_unpack_vector_int128
> function, but his second argument was not the constant 0 or 1.  The compiler
> put the 2nd argument into a register, but there wasn't a valid insn for this,
> and raised an insn not found message.  GCC should warn about this illegal
> usage.

Error, not warn (all the code is correct though).

> 	* config/rs6000/rs6000.c (rs6000_expand_binop_builtin): Insure
> 	that the second argument of the built-in functions to unpack
> 	128-bit scalar types to 64-bit values is 0 or 1.  Change to use a
> 	switch statement instead a lot of if statements.

It usually is easier to review if you post the big mechanical changes
as a separate patch.  But I'll manage, this one isn't so bad :-)

> @@ -14050,6 +14051,21 @@ rs6000_expand_binop_builtin (enum insn_c
>  	  error ("argument 2 must be a 7-bit unsigned literal");
>  	  return CONST0_RTX (tmode);
>  	}
> +      break;
> +    case CODE_FOR_unpackv1ti:
> +    case CODE_FOR_unpackkf:
> +    case CODE_FOR_unpacktf:
> +    case CODE_FOR_unpackif:
> +    case CODE_FOR_unpacktd:
> +      /* Only allow 1-bit unsigned literals. */
> +      STRIP_NOPS (arg1);
> +      if (TREE_CODE (arg1) != INTEGER_CST
> +	  || !IN_RANGE (TREE_INT_CST_LOW (arg1), 0, 1))
> +	{
> +	  error ("argument 2 must be 0 or 1");
> +	  return CONST0_RTX (tmode);
> +	}
> +      break;

This loses that it must be a literal, compared to the 5/6/7 bit messages.
Maybe just say "1-bit unsigned literal", it reads a little bit funny, but
at least it is correct (for some meaning of "literal", anyway) ;-)

Okay for trunk; okay for the release branches with the obvious changes.
Thanks!


Segher
Michael Meissner Aug. 30, 2017, 6:12 p.m. UTC | #2 
Bill Seurer pointed out to me that when I checked in the PR 82015 patches, I
had changed the error message, but I didn't update the test.  I checked this
patch in as obvious:

2017-08-30  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/82015
	* gcc.target/powerpc/pr82015.c: Fix up error message.

Index: gcc/testsuite/gcc.target/powerpc/pr82015.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr82015.c	(revision 251538)
+++ gcc/testsuite/gcc.target/powerpc/pr82015.c	(working copy)
@@ -5,10 +5,10 @@
 
 unsigned long foo_11(vector __int128_t *p)
 {
-  return __builtin_unpack_vector_int128(*p, 11); /* { dg-error "argument 2 must be 0 or 1" } */
+  return __builtin_unpack_vector_int128(*p, 11); /* { dg-error "argument 2 must be a 1-bit unsigned literal" } */
 }
 
 unsigned long foo_n(vector __int128_t *p, unsigned long n)
 {
-  return __builtin_unpack_vector_int128(*p, n);	/* { dg-error "argument 2 must be 0 or 1" } */
+  return __builtin_unpack_vector_int128(*p, n);	/* { dg-error "argument 2 must be a 1-bit unsigned literal" } */
 }
Andreas Schwab Sept. 8, 2017, 7:08 p.m. UTC | #3 
FAIL: gcc.target/powerpc/pr82015.c  (test for errors, line 8)
FAIL: gcc.target/powerpc/pr82015.c  (test for errors, line 13)
FAIL: gcc.target/powerpc/pr82015.c (test for excess errors)
Excess errors:
/daten/gcc/gcc-20170907/gcc/testsuite/gcc.target/powerpc/pr82015.c:6:22: error: unknown type name 'vector'
/daten/gcc/gcc-20170907/gcc/testsuite/gcc.target/powerpc/pr82015.c:11:21: error: unknown type name 'vector'

Andreas.
diff mbox series

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 251390)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -14001,14 +14001,17 @@  rs6000_expand_binop_builtin (enum insn_c
   if (arg0 == error_mark_node || arg1 == error_mark_node)
     return const0_rtx;
 
-  if (icode == CODE_FOR_altivec_vcfux
-      || icode == CODE_FOR_altivec_vcfsx
-      || icode == CODE_FOR_altivec_vctsxs
-      || icode == CODE_FOR_altivec_vctuxs
-      || icode == CODE_FOR_altivec_vspltb
-      || icode == CODE_FOR_altivec_vsplth
-      || icode == CODE_FOR_altivec_vspltw)
+  switch (icode)
     {
+    default:
+      break;
+    case CODE_FOR_altivec_vcfux:
+    case CODE_FOR_altivec_vcfsx:
+    case CODE_FOR_altivec_vctsxs:
+    case CODE_FOR_altivec_vctuxs:
+    case CODE_FOR_altivec_vspltb:
+    case CODE_FOR_altivec_vsplth:
+    case CODE_FOR_altivec_vspltw:
       /* Only allow 5-bit unsigned literals.  */
       STRIP_NOPS (arg1);
       if (TREE_CODE (arg1) != INTEGER_CST
@@ -14017,16 +14020,15 @@  rs6000_expand_binop_builtin (enum insn_c
 	  error ("argument 2 must be a 5-bit unsigned literal");
 	  return CONST0_RTX (tmode);
 	}
-    }
-  else if (icode == CODE_FOR_dfptstsfi_eq_dd
-      || icode == CODE_FOR_dfptstsfi_lt_dd
-      || icode == CODE_FOR_dfptstsfi_gt_dd
-      || icode == CODE_FOR_dfptstsfi_unordered_dd
-      || icode == CODE_FOR_dfptstsfi_eq_td
-      || icode == CODE_FOR_dfptstsfi_lt_td
-      || icode == CODE_FOR_dfptstsfi_gt_td
-      || icode == CODE_FOR_dfptstsfi_unordered_td)
-    {
+      break;
+    case CODE_FOR_dfptstsfi_eq_dd:
+    case CODE_FOR_dfptstsfi_lt_dd:
+    case CODE_FOR_dfptstsfi_gt_dd:
+    case CODE_FOR_dfptstsfi_unordered_dd:
+    case CODE_FOR_dfptstsfi_eq_td:
+    case CODE_FOR_dfptstsfi_lt_td:
+    case CODE_FOR_dfptstsfi_gt_td:
+    case CODE_FOR_dfptstsfi_unordered_td:
       /* Only allow 6-bit unsigned literals.  */
       STRIP_NOPS (arg0);
       if (TREE_CODE (arg0) != INTEGER_CST
@@ -14035,13 +14037,12 @@  rs6000_expand_binop_builtin (enum insn_c
 	  error ("argument 1 must be a 6-bit unsigned literal");
 	  return CONST0_RTX (tmode);
 	}
-    }
-  else if (icode == CODE_FOR_xststdcqp
-	   || icode == CODE_FOR_xststdcdp
-	   || icode == CODE_FOR_xststdcsp
-	   || icode == CODE_FOR_xvtstdcdp
-	   || icode == CODE_FOR_xvtstdcsp)
-    {
+      break;
+    case CODE_FOR_xststdcqp:
+    case CODE_FOR_xststdcdp:
+    case CODE_FOR_xststdcsp:
+    case CODE_FOR_xvtstdcdp:
+    case CODE_FOR_xvtstdcsp:
       /* Only allow 7-bit unsigned literals. */
       STRIP_NOPS (arg1);
       if (TREE_CODE (arg1) != INTEGER_CST
@@ -14050,6 +14051,21 @@  rs6000_expand_binop_builtin (enum insn_c
 	  error ("argument 2 must be a 7-bit unsigned literal");
 	  return CONST0_RTX (tmode);
 	}
+      break;
+    case CODE_FOR_unpackv1ti:
+    case CODE_FOR_unpackkf:
+    case CODE_FOR_unpacktf:
+    case CODE_FOR_unpackif:
+    case CODE_FOR_unpacktd:
+      /* Only allow 1-bit unsigned literals. */
+      STRIP_NOPS (arg1);
+      if (TREE_CODE (arg1) != INTEGER_CST
+	  || !IN_RANGE (TREE_INT_CST_LOW (arg1), 0, 1))
+	{
+	  error ("argument 2 must be 0 or 1");
+	  return CONST0_RTX (tmode);
+	}
+      break;
     }
 
   if (target == 0
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 251390)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -14165,7 +14165,7 @@  (define_insn_and_split "pack<mode>"
    (set_attr "length" "4,8")])
 
 (define_insn "unpack<mode>"
-  [(set (match_operand:DI 0 "register_operand" "=d,d")
+  [(set (match_operand:DI 0 "register_operand" "=wa,wa")
 	(unspec:DI [(match_operand:FMOVE128_VSX 1 "register_operand" "0,wa")
 		    (match_operand:QI 2 "const_0_to_1_operand" "O,i")]
 	 UNSPEC_UNPACK_128BIT))]
@@ -14182,8 +14182,8 @@  (define_insn "unpack<mode>"
 (define_insn "pack<mode>"
   [(set (match_operand:FMOVE128_VSX 0 "register_operand" "=wa")
 	(unspec:FMOVE128_VSX
-	 [(match_operand:DI 1 "register_operand" "d")
-	  (match_operand:DI 2 "register_operand" "d")]
+	 [(match_operand:DI 1 "register_operand" "wa")
+	  (match_operand:DI 2 "register_operand" "wa")]
 	 UNSPEC_PACK_128BIT))]
   "TARGET_VSX"
   "xxpermdi %x0,%x1,%x2,0"
Index: gcc/testsuite/gcc.target/powerpc/pr82015.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr82015.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr82015.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-O2 -mvsx" } */
+
+unsigned long foo_11(vector __int128_t *p)
+{
+  return __builtin_unpack_vector_int128(*p, 11); /* { dg-error "argument 2 must be 0 or 1" } */
+}
+
+unsigned long foo_n(vector __int128_t *p, unsigned long n)
+{
+  return __builtin_unpack_vector_int128(*p, n);	/* { dg-error "argument 2 must be 0 or 1" } */
+}