* [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument
@ 2017-08-29 6:57 Michael Meissner
2017-08-29 14:13 ` Segher Boessenkool
2017-09-08 19:08 ` Andreas Schwab
0 siblings, 2 replies; 4+ messages in thread
From: Michael Meissner @ 2017-08-29 6:57 UTC (permalink / raw)
To: GCC Patches, Segher Boessenkool, David Edelsohn, Bill Schmidt
[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]
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.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797
[-- Attachment #2: pr82015.patch01b --]
[-- Type: text/plain, Size: 5083 bytes --]
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" } */
+}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument
2017-08-29 6:57 [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument Michael Meissner
@ 2017-08-29 14:13 ` Segher Boessenkool
2017-08-30 19:29 ` Michael Meissner
2017-09-08 19:08 ` Andreas Schwab
1 sibling, 1 reply; 4+ messages in thread
From: Segher Boessenkool @ 2017-08-29 14:13 UTC (permalink / raw)
To: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument
2017-08-29 14:13 ` Segher Boessenkool
@ 2017-08-30 19:29 ` Michael Meissner
0 siblings, 0 replies; 4+ messages in thread
From: Michael Meissner @ 2017-08-30 19:29 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt
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" } */
}
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument
2017-08-29 6:57 [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument Michael Meissner
2017-08-29 14:13 ` Segher Boessenkool
@ 2017-09-08 19:08 ` Andreas Schwab
1 sibling, 0 replies; 4+ messages in thread
From: Andreas Schwab @ 2017-09-08 19:08 UTC (permalink / raw)
To: Michael Meissner
Cc: GCC Patches, Segher Boessenkool, David Edelsohn, Bill Schmidt
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.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-08 19:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-29 6:57 [PATCH], PR target/82015, add PowerPC warning for unpack_vector_int128 with illegal 2nd argument Michael Meissner
2017-08-29 14:13 ` Segher Boessenkool
2017-08-30 19:29 ` Michael Meissner
2017-09-08 19:08 ` Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).