* [PATCH, 5.1, rs6000] Fix PR65787
@ 2015-04-16 21:46 Bill Schmidt
2015-04-17 12:27 ` Bill Schmidt
0 siblings, 1 reply; 13+ messages in thread
From: Bill Schmidt @ 2015-04-16 21:46 UTC (permalink / raw)
To: gcc-patches; +Cc: dje.gcc
Hi,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65787 identifies an issue
where the powerpc64le vector swap optimization miscompiles some code.
The code for handling vector extract operations did not expect to find
those operations wrapped in a PARALLEL with a CLOBBER, but this test
shows that this can now happen. This patch adds that possibility to the
handling for vector extract. I've added a small test case to verify
that we now catch this.
Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
regressions. Is this ok for trunk and gcc-5-branch?
After this is complete I will investigate whether we need to backport
this to 4.8 and 4.9 also.
Thanks,
Bill
[gcc]
2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
PR target/65787
* config/rs6000/rs6000.c (rtx_is_swappable_p): Handle case where
vec_extract operation is wrapped in a PARALLEL with a CLOBBER.
(adjust_extract): Likewise.
[gcc/testsuite]
2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
PR target/65787
* gcc.target/powerpc/pr65787.c: New.
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 222158)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -34204,6 +34204,20 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
else
return 0;
+ case PARALLEL: {
+ /* A vec_extract operation may be wrapped in a PARALLEL with a
+ clobber, so account for that possibility. */
+ unsigned int len = XVECLEN (op, 0);
+
+ if (len != 2)
+ return 0;
+
+ if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
+ return 0;
+
+ return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
+ }
+
case UNSPEC:
{
/* Various operations are unsafe for this optimization, at least
@@ -34603,7 +34617,10 @@ permute_store (rtx_insn *insn)
static void
adjust_extract (rtx_insn *insn)
{
- rtx src = SET_SRC (PATTERN (insn));
+ rtx pattern = PATTERN (insn);
+ if (GET_CODE (pattern) == PARALLEL)
+ pattern = XVECEXP (pattern, 0, 0);
+ rtx src = SET_SRC (pattern);
/* The vec_select may be wrapped in a vec_duplicate for a splat, so
account for that. */
rtx sel = GET_CODE (src) == VEC_DUPLICATE ? XEXP (src, 0) : src;
Index: gcc/testsuite/gcc.target/powerpc/pr65787.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr65787.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr65787.c (working copy)
@@ -0,0 +1,21 @@
+/* { dg-do compile { target { powerpc64le-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
+/* { dg-options "-mcpu=power8 -O3" } */
+/* { dg-final { scan-assembler "xxsldwi \[0-9\]*,\[0-9\]*,\[0-9\]*,3" } } */
+/* { dg-final { scan-assembler-not "xxpermdi" } } */
+
+/* This test verifies that a vector extract operand properly has its
+ lane changed by the swap optimization. Element 2 of LE corresponds
+ to element 1 of BE. When doublewords are swapped, this becomes
+ element 3 of BE, so we need to shift the vector left by 3 words
+ to be able to extract the correct value from BE element zero. */
+
+typedef float v4f32 __attribute__ ((__vector_size__ (16)));
+
+void foo (float);
+extern v4f32 x, y;
+
+int main() {
+ v4f32 z = x + y;
+ foo (z[2]);
+}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-16 21:46 [PATCH, 5.1, rs6000] Fix PR65787 Bill Schmidt
@ 2015-04-17 12:27 ` Bill Schmidt
2015-04-17 13:28 ` Bill Schmidt
0 siblings, 1 reply; 13+ messages in thread
From: Bill Schmidt @ 2015-04-17 12:27 UTC (permalink / raw)
To: gcc-patches; +Cc: dje.gcc
Note that Jakub requested a small change in the bugzilla commentary,
which I've implemented. I'm doing a regstrap now.
Bill
On Thu, 2015-04-16 at 16:46 -0500, Bill Schmidt wrote:
> Hi,
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65787 identifies an issue
> where the powerpc64le vector swap optimization miscompiles some code.
> The code for handling vector extract operations did not expect to find
> those operations wrapped in a PARALLEL with a CLOBBER, but this test
> shows that this can now happen. This patch adds that possibility to the
> handling for vector extract. I've added a small test case to verify
> that we now catch this.
>
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> regressions. Is this ok for trunk and gcc-5-branch?
>
> After this is complete I will investigate whether we need to backport
> this to 4.8 and 4.9 also.
>
> Thanks,
> Bill
>
>
> [gcc]
>
> 2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
>
> PR target/65787
> * config/rs6000/rs6000.c (rtx_is_swappable_p): Handle case where
> vec_extract operation is wrapped in a PARALLEL with a CLOBBER.
> (adjust_extract): Likewise.
>
> [gcc/testsuite]
>
> 2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
>
> PR target/65787
> * gcc.target/powerpc/pr65787.c: New.
>
>
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c (revision 222158)
> +++ gcc/config/rs6000/rs6000.c (working copy)
> @@ -34204,6 +34204,20 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
> else
> return 0;
>
> + case PARALLEL: {
> + /* A vec_extract operation may be wrapped in a PARALLEL with a
> + clobber, so account for that possibility. */
> + unsigned int len = XVECLEN (op, 0);
> +
> + if (len != 2)
> + return 0;
> +
> + if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
> + return 0;
> +
> + return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
> + }
> +
> case UNSPEC:
> {
> /* Various operations are unsafe for this optimization, at least
> @@ -34603,7 +34617,10 @@ permute_store (rtx_insn *insn)
> static void
> adjust_extract (rtx_insn *insn)
> {
> - rtx src = SET_SRC (PATTERN (insn));
> + rtx pattern = PATTERN (insn);
> + if (GET_CODE (pattern) == PARALLEL)
> + pattern = XVECEXP (pattern, 0, 0);
> + rtx src = SET_SRC (pattern);
> /* The vec_select may be wrapped in a vec_duplicate for a splat, so
> account for that. */
> rtx sel = GET_CODE (src) == VEC_DUPLICATE ? XEXP (src, 0) : src;
> Index: gcc/testsuite/gcc.target/powerpc/pr65787.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/pr65787.c (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/pr65787.c (working copy)
> @@ -0,0 +1,21 @@
> +/* { dg-do compile { target { powerpc64le-*-* } } } */
> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
> +/* { dg-options "-mcpu=power8 -O3" } */
> +/* { dg-final { scan-assembler "xxsldwi \[0-9\]*,\[0-9\]*,\[0-9\]*,3" } } */
> +/* { dg-final { scan-assembler-not "xxpermdi" } } */
> +
> +/* This test verifies that a vector extract operand properly has its
> + lane changed by the swap optimization. Element 2 of LE corresponds
> + to element 1 of BE. When doublewords are swapped, this becomes
> + element 3 of BE, so we need to shift the vector left by 3 words
> + to be able to extract the correct value from BE element zero. */
> +
> +typedef float v4f32 __attribute__ ((__vector_size__ (16)));
> +
> +void foo (float);
> +extern v4f32 x, y;
> +
> +int main() {
> + v4f32 z = x + y;
> + foo (z[2]);
> +}
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 12:27 ` Bill Schmidt
@ 2015-04-17 13:28 ` Bill Schmidt
2015-04-17 14:42 ` David Edelsohn
2015-04-17 14:49 ` Jakub Jelinek
0 siblings, 2 replies; 13+ messages in thread
From: Bill Schmidt @ 2015-04-17 13:28 UTC (permalink / raw)
To: gcc-patches; +Cc: dje.gcc
On Fri, 2015-04-17 at 07:27 -0500, Bill Schmidt wrote:
> Note that Jakub requested a small change in the bugzilla commentary,
> which I've implemented. I'm doing a regstrap now.
>
> Bill
>
Here's the revised and tested patch. OK for trunk and gcc-5-branch?
Thanks,
Bill
[gcc]
2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
PR target/65787
* config/rs6000/rs6000.c (rtx_is_swappable_p): Handle case where
vec_extract operation is wrapped in a PARALLEL with a CLOBBER.
(adjust_extract): Likewise.
[gcc/testsuite]
2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
PR target/65787
* gcc.target/powerpc/pr65787.c: New.
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 222158)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -34204,6 +34204,20 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
else
return 0;
+ case PARALLEL: {
+ /* A vec_extract operation may be wrapped in a PARALLEL with a
+ clobber, so account for that possibility. */
+ unsigned int len = XVECLEN (op, 0);
+
+ if (len != 2)
+ return 0;
+
+ if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
+ return 0;
+
+ return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
+ }
+
case UNSPEC:
{
/* Various operations are unsafe for this optimization, at least
@@ -34603,7 +34617,10 @@ permute_store (rtx_insn *insn)
static void
adjust_extract (rtx_insn *insn)
{
- rtx src = SET_SRC (PATTERN (insn));
+ rtx pattern = PATTERN (insn);
+ if (GET_CODE (pattern) == PARALLEL)
+ pattern = XVECEXP (pattern, 0, 0);
+ rtx src = SET_SRC (pattern);
/* The vec_select may be wrapped in a vec_duplicate for a splat, so
account for that. */
rtx sel = GET_CODE (src) == VEC_DUPLICATE ? XEXP (src, 0) : src;
Index: gcc/testsuite/gcc.target/powerpc/pr65787.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr65787.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr65787.c (working copy)
@@ -0,0 +1,21 @@
+/* { dg-do compile { target { powerpc64le-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
+/* { dg-options "-mcpu=power8 -O3" } */
+/* { dg-final { scan-assembler "xxsldwi \[0-9\]*,\[0-9\]*,\[0-9\]*,3" } } */
+/* { dg-final { scan-assembler-not "xxpermdi" } } */
+
+/* This test verifies that a vector extract operand properly has its
+ lane changed by the swap optimization. Element 2 of LE corresponds
+ to element 1 of BE. When doublewords are swapped, this becomes
+ element 3 of BE, so we need to shift the vector left by 3 words
+ to be able to extract the correct value from BE element zero. */
+
+typedef float v4f32 __attribute__ ((__vector_size__ (16)));
+
+void foo (float);
+extern v4f32 x, y;
+
+int main() {
+ v4f32 z = x + y;
+ foo (z[2]);
+}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 13:28 ` Bill Schmidt
@ 2015-04-17 14:42 ` David Edelsohn
2015-04-17 14:49 ` Jakub Jelinek
1 sibling, 0 replies; 13+ messages in thread
From: David Edelsohn @ 2015-04-17 14:42 UTC (permalink / raw)
To: Bill Schmidt; +Cc: GCC Patches
On Fri, Apr 17, 2015 at 9:28 AM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> On Fri, 2015-04-17 at 07:27 -0500, Bill Schmidt wrote:
>> Note that Jakub requested a small change in the bugzilla commentary,
>> which I've implemented. I'm doing a regstrap now.
>>
>> Bill
>>
>
> Here's the revised and tested patch. OK for trunk and gcc-5-branch?
>
> Thanks,
> Bill
>
>
> [gcc]
>
> 2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
>
> PR target/65787
> * config/rs6000/rs6000.c (rtx_is_swappable_p): Handle case where
> vec_extract operation is wrapped in a PARALLEL with a CLOBBER.
> (adjust_extract): Likewise.
>
> [gcc/testsuite]
>
> 2015-04-16 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
>
> PR target/65787
> * gcc.target/powerpc/pr65787.c: New.
The revised patch is okay.
Thanks, David
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 13:28 ` Bill Schmidt
2015-04-17 14:42 ` David Edelsohn
@ 2015-04-17 14:49 ` Jakub Jelinek
2015-04-17 15:02 ` Bill Schmidt
1 sibling, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2015-04-17 14:49 UTC (permalink / raw)
To: Bill Schmidt; +Cc: gcc-patches, dje.gcc
On Fri, Apr 17, 2015 at 08:28:02AM -0500, Bill Schmidt wrote:
> On Fri, 2015-04-17 at 07:27 -0500, Bill Schmidt wrote:
> > Note that Jakub requested a small change in the bugzilla commentary,
> > which I've implemented. I'm doing a regstrap now.
> >
> > Bill
> >
>
> Here's the revised and tested patch. OK for trunk and gcc-5-branch?
You have actually mailed the original patch again, not the revised one.
That said, PARALLEL seems to be already handled by rtx_is_swappable_p,
so if it isn't handled correctly, perhaps there is a bug in that function.
for (i = 0; i < GET_RTX_LENGTH (code); ++i)
if (fmt[i] == 'e' || fmt[i] == 'u')
{
unsigned int special_op = SH_NONE;
ok &= rtx_is_swappable_p (XEXP (op, i), &special_op);
/* Ensure we never have two kinds of special handling
for the same insn. */
if (*special != SH_NONE && special_op != SH_NONE
&& *special != special_op)
return 0;
*special = special_op;
}
else if (fmt[i] == 'E')
for (j = 0; j < XVECLEN (op, i); ++j)
{
unsigned int special_op = SH_NONE;
ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
/* Ensure we never have two kinds of special handling
for the same insn. */
if (*special != SH_NONE && special_op != SH_NONE
&& *special != special_op)
return 0;
*special = special_op;
}
If the comments are correct, then supposedly if say on the PARALLEL with
a SET that is SH_EXTRACT and a CLOBBER that is SH_NONE, both returning 1,
the outcome should by return 1 (that is the case), but *special should be
SH_EXTRACT, rather than the last case wins right now (SH_NONE).
If so, then perhaps after each of the ok &= rtx_is_swappable_p ...
line should be
if (special_op == SH_NONE)
continue;
so that we never store SH_NONE (leave that just to the initial store), and
then just
if (*special != SH_NONE && *special != special_op)
return 0;
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 14:49 ` Jakub Jelinek
@ 2015-04-17 15:02 ` Bill Schmidt
2015-04-17 16:32 ` Bill Schmidt
0 siblings, 1 reply; 13+ messages in thread
From: Bill Schmidt @ 2015-04-17 15:02 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, dje.gcc
On Fri, 2015-04-17 at 16:49 +0200, Jakub Jelinek wrote:
> On Fri, Apr 17, 2015 at 08:28:02AM -0500, Bill Schmidt wrote:
> > On Fri, 2015-04-17 at 07:27 -0500, Bill Schmidt wrote:
> > > Note that Jakub requested a small change in the bugzilla commentary,
> > > which I've implemented. I'm doing a regstrap now.
> > >
> > > Bill
> > >
> >
> > Here's the revised and tested patch. OK for trunk and gcc-5-branch?
>
> You have actually mailed the original patch again, not the revised one.
Yes, sorry, I had just noticed that. I forgot to download the revised
patch to the system I send my mail from. This is what I get for
multitasking during a meeting...
>
> That said, PARALLEL seems to be already handled by rtx_is_swappable_p,
> so if it isn't handled correctly, perhaps there is a bug in that function.
>
> for (i = 0; i < GET_RTX_LENGTH (code); ++i)
> if (fmt[i] == 'e' || fmt[i] == 'u')
> {
> unsigned int special_op = SH_NONE;
> ok &= rtx_is_swappable_p (XEXP (op, i), &special_op);
> /* Ensure we never have two kinds of special handling
> for the same insn. */
> if (*special != SH_NONE && special_op != SH_NONE
> && *special != special_op)
> return 0;
> *special = special_op;
> }
> else if (fmt[i] == 'E')
> for (j = 0; j < XVECLEN (op, i); ++j)
> {
> unsigned int special_op = SH_NONE;
> ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
> /* Ensure we never have two kinds of special handling
> for the same insn. */
> if (*special != SH_NONE && special_op != SH_NONE
> && *special != special_op)
> return 0;
> *special = special_op;
> }
>
> If the comments are correct, then supposedly if say on the PARALLEL with
> a SET that is SH_EXTRACT and a CLOBBER that is SH_NONE, both returning 1,
> the outcome should by return 1 (that is the case), but *special should be
> SH_EXTRACT, rather than the last case wins right now (SH_NONE).
> If so, then perhaps after each of the ok &= rtx_is_swappable_p ...
> line should be
> if (special_op == SH_NONE)
> continue;
> so that we never store SH_NONE (leave that just to the initial store), and
> then just
> if (*special != SH_NONE && *special != special_op)
> return 0;
Hm, yes, there is definitely a problem here. Let me look at reworking
this. Thanks!
Bill
>
> Jakub
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 15:02 ` Bill Schmidt
@ 2015-04-17 16:32 ` Bill Schmidt
2015-04-17 16:40 ` Jakub Jelinek
0 siblings, 1 reply; 13+ messages in thread
From: Bill Schmidt @ 2015-04-17 16:32 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, dje.gcc
Hi,
On Fri, 2015-04-17 at 10:02 -0500, Bill Schmidt wrote:
> On Fri, 2015-04-17 at 16:49 +0200, Jakub Jelinek wrote:
> > You have actually mailed the original patch again, not the revised one.
>
> > That said, PARALLEL seems to be already handled by rtx_is_swappable_p,
> > so if it isn't handled correctly, perhaps there is a bug in that function.
> >
Quite right. I've fixed this as you suggested in the attached patch.
Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
Is this version ok?
Sorry for apparently not knowing my own code as well as I should... :/
Thanks,
Bill
2015-04-17 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
PR target/65787
* config/rs6000/rs6000.c (rtx_is_swappable_p): Remove previous
fix; ensure that a subsequent SH_NONE operand does not overwrite
an existing *special value.
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 222182)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -34204,17 +34204,6 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
else
return 0;
- case PARALLEL:
- /* A vec_extract operation may be wrapped in a PARALLEL with a
- clobber, so account for that possibility. */
- if (XVECLEN (op, 0) != 2)
- return 0;
-
- if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
- return 0;
-
- return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
-
case UNSPEC:
{
/* Various operations are unsafe for this optimization, at least
@@ -34308,6 +34297,8 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
{
unsigned int special_op = SH_NONE;
ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
+ if (special_op == SH_NONE)
+ continue;
/* Ensure we never have two kinds of special handling
for the same insn. */
if (*special != SH_NONE && special_op != SH_NONE
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 16:32 ` Bill Schmidt
@ 2015-04-17 16:40 ` Jakub Jelinek
2015-04-17 17:47 ` Jakub Jelinek
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2015-04-17 16:40 UTC (permalink / raw)
To: Bill Schmidt; +Cc: gcc-patches, dje.gcc
On Fri, Apr 17, 2015 at 11:32:44AM -0500, Bill Schmidt wrote:
> 2015-04-17 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
>
> PR target/65787
> * config/rs6000/rs6000.c (rtx_is_swappable_p): Remove previous
> fix; ensure that a subsequent SH_NONE operand does not overwrite
> an existing *special value.
>
>
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c (revision 222182)
> +++ gcc/config/rs6000/rs6000.c (working copy)
> @@ -34204,17 +34204,6 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
> else
> return 0;
>
> - case PARALLEL:
> - /* A vec_extract operation may be wrapped in a PARALLEL with a
> - clobber, so account for that possibility. */
> - if (XVECLEN (op, 0) != 2)
> - return 0;
> -
> - if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
> - return 0;
> -
> - return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
> -
> case UNSPEC:
> {
> /* Various operations are unsafe for this optimization, at least
> @@ -34308,6 +34297,8 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
> {
> unsigned int special_op = SH_NONE;
> ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
> + if (special_op == SH_NONE)
> + continue;
> /* Ensure we never have two kinds of special handling
> for the same insn. */
> if (*special != SH_NONE && special_op != SH_NONE
The " && special_op != SH_NONE" test from the second if can go then,
because it is never true. And I'd really think that you shouldn't change
just the fmt[i] == 'E' handling, but also the fmt[i] == 'e' || fmt[i] == 'u'
handling a few lines earlier (both the added
"if (special_op == SH_NONE) continue;" there and
removal of " && special_op != SH_NONE".
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 16:40 ` Jakub Jelinek
@ 2015-04-17 17:47 ` Jakub Jelinek
2015-04-17 18:06 ` Bill Schmidt
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2015-04-17 17:47 UTC (permalink / raw)
To: Bill Schmidt; +Cc: gcc-patches, dje.gcc
On Fri, Apr 17, 2015 at 06:39:59PM +0200, Jakub Jelinek wrote:
> The " && special_op != SH_NONE" test from the second if can go then,
> because it is never true. And I'd really think that you shouldn't change
> just the fmt[i] == 'E' handling, but also the fmt[i] == 'e' || fmt[i] == 'u'
> handling a few lines earlier (both the added
> "if (special_op == SH_NONE) continue;" there and
> removal of " && special_op != SH_NONE".
In particular, this is what I had in mind.
2015-04-17 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
PR target/65787
* config/rs6000/rs6000.c (rtx_is_swappable_p): Remove previous
fix; ensure that a subsequent SH_NONE operand does not overwrite
an existing *special value.
--- gcc/config/rs6000/rs6000.c.jj 2015-04-17 19:09:59.000000000 +0200
+++ gcc/config/rs6000/rs6000.c 2015-04-17 19:28:43.264784372 +0200
@@ -34204,17 +34204,6 @@ rtx_is_swappable_p (rtx op, unsigned int
else
return 0;
- case PARALLEL:
- /* A vec_extract operation may be wrapped in a PARALLEL with a
- clobber, so account for that possibility. */
- if (XVECLEN (op, 0) != 2)
- return 0;
-
- if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
- return 0;
-
- return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
-
case UNSPEC:
{
/* Various operations are unsafe for this optimization, at least
@@ -34296,10 +34285,11 @@ rtx_is_swappable_p (rtx op, unsigned int
{
unsigned int special_op = SH_NONE;
ok &= rtx_is_swappable_p (XEXP (op, i), &special_op);
+ if (special_op == SH_NONE)
+ continue;
/* Ensure we never have two kinds of special handling
for the same insn. */
- if (*special != SH_NONE && special_op != SH_NONE
- && *special != special_op)
+ if (*special != SH_NONE && *special != special_op)
return 0;
*special = special_op;
}
@@ -34308,10 +34298,11 @@ rtx_is_swappable_p (rtx op, unsigned int
{
unsigned int special_op = SH_NONE;
ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
+ if (special_op == SH_NONE)
+ continue;
/* Ensure we never have two kinds of special handling
for the same insn. */
- if (*special != SH_NONE && special_op != SH_NONE
- && *special != special_op)
+ if (*special != SH_NONE && *special != special_op)
return 0;
*special = special_op;
}
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 17:47 ` Jakub Jelinek
@ 2015-04-17 18:06 ` Bill Schmidt
2015-04-17 18:27 ` Jakub Jelinek
0 siblings, 1 reply; 13+ messages in thread
From: Bill Schmidt @ 2015-04-17 18:06 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, dje.gcc
Yep, thanks -- I just finished testing that, and it fixes the problem
with no regressions. Thanks for the help.
Is this ok to commit?
Thanks,
Bill
On Fri, 2015-04-17 at 19:46 +0200, Jakub Jelinek wrote:
> On Fri, Apr 17, 2015 at 06:39:59PM +0200, Jakub Jelinek wrote:
> > The " && special_op != SH_NONE" test from the second if can go then,
> > because it is never true. And I'd really think that you shouldn't change
> > just the fmt[i] == 'E' handling, but also the fmt[i] == 'e' || fmt[i] == 'u'
> > handling a few lines earlier (both the added
> > "if (special_op == SH_NONE) continue;" there and
> > removal of " && special_op != SH_NONE".
>
> In particular, this is what I had in mind.
>
> 2015-04-17 Bill Schmidt <wschmidt@linux.vnet.ibm.com>
>
> PR target/65787
> * config/rs6000/rs6000.c (rtx_is_swappable_p): Remove previous
> fix; ensure that a subsequent SH_NONE operand does not overwrite
> an existing *special value.
>
> --- gcc/config/rs6000/rs6000.c.jj 2015-04-17 19:09:59.000000000 +0200
> +++ gcc/config/rs6000/rs6000.c 2015-04-17 19:28:43.264784372 +0200
> @@ -34204,17 +34204,6 @@ rtx_is_swappable_p (rtx op, unsigned int
> else
> return 0;
>
> - case PARALLEL:
> - /* A vec_extract operation may be wrapped in a PARALLEL with a
> - clobber, so account for that possibility. */
> - if (XVECLEN (op, 0) != 2)
> - return 0;
> -
> - if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
> - return 0;
> -
> - return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
> -
> case UNSPEC:
> {
> /* Various operations are unsafe for this optimization, at least
> @@ -34296,10 +34285,11 @@ rtx_is_swappable_p (rtx op, unsigned int
> {
> unsigned int special_op = SH_NONE;
> ok &= rtx_is_swappable_p (XEXP (op, i), &special_op);
> + if (special_op == SH_NONE)
> + continue;
> /* Ensure we never have two kinds of special handling
> for the same insn. */
> - if (*special != SH_NONE && special_op != SH_NONE
> - && *special != special_op)
> + if (*special != SH_NONE && *special != special_op)
> return 0;
> *special = special_op;
> }
> @@ -34308,10 +34298,11 @@ rtx_is_swappable_p (rtx op, unsigned int
> {
> unsigned int special_op = SH_NONE;
> ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
> + if (special_op == SH_NONE)
> + continue;
> /* Ensure we never have two kinds of special handling
> for the same insn. */
> - if (*special != SH_NONE && special_op != SH_NONE
> - && *special != special_op)
> + if (*special != SH_NONE && *special != special_op)
> return 0;
> *special = special_op;
> }
>
>
> Jakub
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 18:06 ` Bill Schmidt
@ 2015-04-17 18:27 ` Jakub Jelinek
2015-04-17 19:00 ` David Edelsohn
2015-04-17 20:29 ` Bill Schmidt
0 siblings, 2 replies; 13+ messages in thread
From: Jakub Jelinek @ 2015-04-17 18:27 UTC (permalink / raw)
To: Bill Schmidt; +Cc: gcc-patches, dje.gcc
On Fri, Apr 17, 2015 at 01:06:22PM -0500, Bill Schmidt wrote:
> Yep, thanks -- I just finished testing that, and it fixes the problem
> with no regressions. Thanks for the help.
>
> Is this ok to commit?
If David is ok with it, it is fine with me too. But, please commit to both
gcc-5-branch and the trunk, your last commit only went to the branch and not
to the trunk.
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 18:27 ` Jakub Jelinek
@ 2015-04-17 19:00 ` David Edelsohn
2015-04-17 20:29 ` Bill Schmidt
1 sibling, 0 replies; 13+ messages in thread
From: David Edelsohn @ 2015-04-17 19:00 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Bill Schmidt, GCC Patches
On Fri, Apr 17, 2015 at 2:27 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Apr 17, 2015 at 01:06:22PM -0500, Bill Schmidt wrote:
>> Yep, thanks -- I just finished testing that, and it fixes the problem
>> with no regressions. Thanks for the help.
>>
>> Is this ok to commit?
>
> If David is ok with it, it is fine with me too. But, please commit to both
> gcc-5-branch and the trunk, your last commit only went to the branch and not
> to the trunk.
Fine with me too.
And thanks a lot to Jakub for helping design the correct solution.
Thanks, David
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, 5.1, rs6000] Fix PR65787
2015-04-17 18:27 ` Jakub Jelinek
2015-04-17 19:00 ` David Edelsohn
@ 2015-04-17 20:29 ` Bill Schmidt
1 sibling, 0 replies; 13+ messages in thread
From: Bill Schmidt @ 2015-04-17 20:29 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, dje.gcc
On Fri, 2015-04-17 at 20:27 +0200, Jakub Jelinek wrote:
> On Fri, Apr 17, 2015 at 01:06:22PM -0500, Bill Schmidt wrote:
> > Yep, thanks -- I just finished testing that, and it fixes the problem
> > with no regressions. Thanks for the help.
> >
> > Is this ok to commit?
>
> If David is ok with it, it is fine with me too. But, please commit to both
> gcc-5-branch and the trunk, your last commit only went to the branch and not
> to the trunk.
Yes, absolutely. I was about to push it to trunk when I found out it
was inadequate, so I held off. I'll push the combined correct patch
there as well.
Thank you again for your help!
Bill
>
> Jakub
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-04-17 20:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-16 21:46 [PATCH, 5.1, rs6000] Fix PR65787 Bill Schmidt
2015-04-17 12:27 ` Bill Schmidt
2015-04-17 13:28 ` Bill Schmidt
2015-04-17 14:42 ` David Edelsohn
2015-04-17 14:49 ` Jakub Jelinek
2015-04-17 15:02 ` Bill Schmidt
2015-04-17 16:32 ` Bill Schmidt
2015-04-17 16:40 ` Jakub Jelinek
2015-04-17 17:47 ` Jakub Jelinek
2015-04-17 18:06 ` Bill Schmidt
2015-04-17 18:27 ` Jakub Jelinek
2015-04-17 19:00 ` David Edelsohn
2015-04-17 20:29 ` Bill Schmidt
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).