* [PATCH] Fix a typo in two_value_replacement function
@ 2019-05-05 6:31 Li Jia He
2019-05-05 9:48 ` Jakub Jelinek
2019-05-06 11:19 ` Christophe Lyon
0 siblings, 2 replies; 8+ messages in thread
From: Li Jia He @ 2019-05-05 6:31 UTC (permalink / raw)
To: gcc-patches, segher, wschmidt, rguenther, jakub; +Cc: Li Jia He
Hi,
GCC revision 267634 implemented two_value_replacement function.
However, a typo occurred during the parameter check, which caused
us to miss some optimizations.
The intent of the code might be to check that the input parameters
are const int and their difference is one. However, when I read
the code, I found that it is wrong to detect whether an input data
plus one is equal to itself. This could be a typo.
The regression testing for the patch was done on GCC mainline on
powerpc64le-unknown-linux-gnu (Power 9 LE)
with no regressions. Is it OK for trunk and backport to gcc 9 ?
Thanks,
Lijia He
gcc/ChangeLog
2019-05-05 Li Jia He <helijia@linux.ibm.com>
* tree-ssa-phiopt.c (two_value_replacement):
Fix a typo in parameter detection.
gcc/testsuite/ChangeLog
2019-05-05 Li Jia He <helijia@linux.ibm.com>
* gcc.dg/pr88676.c: Modify the include header file.
* gcc.dg/tree-ssa/pr37508.c: Add the no-ssa-phiopt option to
skip phi optimization.
* gcc.dg/tree-ssa/pr88676.c: Rename to ...
* gcc.dg/tree-ssa/pr88676-1.c: ... this new file.
* gcc.dg/tree-ssa/pr88676-2.c: New testcase.
---
gcc/testsuite/gcc.dg/pr88676.c | 2 +-
gcc/testsuite/gcc.dg/tree-ssa/pr37508.c | 6 ++--
.../tree-ssa/{pr88676.c => pr88676-1.c} | 0
gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c | 30 +++++++++++++++++++
gcc/tree-ssa-phiopt.c | 2 +-
5 files changed, 35 insertions(+), 5 deletions(-)
rename gcc/testsuite/gcc.dg/tree-ssa/{pr88676.c => pr88676-1.c} (100%)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
diff --git a/gcc/testsuite/gcc.dg/pr88676.c b/gcc/testsuite/gcc.dg/pr88676.c
index b5fdd9342b8..719208083ae 100644
--- a/gcc/testsuite/gcc.dg/pr88676.c
+++ b/gcc/testsuite/gcc.dg/pr88676.c
@@ -2,7 +2,7 @@
/* { dg-do run } */
/* { dg-options "-O2" } */
-#include "tree-ssa/pr88676.c"
+#include "tree-ssa/pr88676-1.c"
__attribute__((noipa)) void
bar (int x, int y, int z)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c b/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
index 2ba09afe481..a6def045de4 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-O2 -fno-tree-fre -fdump-tree-vrp1" } */
+/* { dg-options "-O2 -fno-ssa-phiopt -fno-tree-fre -fdump-tree-vrp1" } */
struct foo1 {
int i:1;
@@ -22,7 +22,7 @@ int test2 (struct foo2 *x)
{
if (x->i == 0)
return 1;
- else if (x->i == -1) /* This test is already folded to false by ccp1. */
+ else if (x->i == -1) /* This test is already optimized by ccp1 or phiopt1. */
return 1;
return 0;
}
@@ -31,7 +31,7 @@ int test3 (struct foo1 *x)
{
if (x->i == 0)
return 1;
- else if (x->i == 1) /* This test is already folded to false by fold. */
+ else if (x->i == 1) /* This test is already optimized by ccp1 or phiopt1. */
return 1;
return 0;
}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88676.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
similarity index 100%
rename from gcc/testsuite/gcc.dg/tree-ssa/pr88676.c
rename to gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
new file mode 100644
index 00000000000..d377948e14d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
@@ -0,0 +1,30 @@
+/* PR tree-optimization/88676 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+/* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
+
+struct foo1 {
+ int i:1;
+};
+struct foo2 {
+ unsigned i:1;
+};
+
+int test1 (struct foo1 *x)
+{
+ if (x->i == 0)
+ return 1;
+ else if (x->i == 1)
+ return 1;
+ return 0;
+}
+
+int test2 (struct foo2 *x)
+{
+ if (x->i == 0)
+ return 1;
+ else if (x->i == -1)
+ return 1;
+ return 0;
+}
+
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 219791ea4ba..90674a2f3c4 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -602,7 +602,7 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
|| TREE_CODE (arg1) != INTEGER_CST
|| (tree_int_cst_lt (arg0, arg1)
? wi::to_widest (arg0) + 1 != wi::to_widest (arg1)
- : wi::to_widest (arg1) + 1 != wi::to_widest (arg1)))
+ : wi::to_widest (arg1) + 1 != wi::to_widest (arg0)))
return false;
if (!empty_block_p (middle_bb))
--
2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-05 6:31 [PATCH] Fix a typo in two_value_replacement function Li Jia He
@ 2019-05-05 9:48 ` Jakub Jelinek
2019-05-06 11:19 ` Christophe Lyon
1 sibling, 0 replies; 8+ messages in thread
From: Jakub Jelinek @ 2019-05-05 9:48 UTC (permalink / raw)
To: Li Jia He; +Cc: gcc-patches, segher, wschmidt, rguenther
On Sun, May 05, 2019 at 01:31:12AM -0500, Li Jia He wrote:
> GCC revision 267634 implemented two_value_replacement function.
> However, a typo occurred during the parameter check, which caused
> us to miss some optimizations.
Thanks for catching this.
> The regression testing for the patch was done on GCC mainline on
>
> powerpc64le-unknown-linux-gnu (Power 9 LE)
>
> with no regressions. Is it OK for trunk and backport to gcc 9 ?
Ok for both, but see below.
> gcc/ChangeLog
>
> 2019-05-05 Li Jia He <helijia@linux.ibm.com>
>
> * tree-ssa-phiopt.c (two_value_replacement):
> Fix a typo in parameter detection.
Don't break a line after :, only when you reach 80 columns. So:
* tree-ssa-phiopt.c (two_value_replacement): Fix a typo in parameter
detection.
> * gcc.dg/pr88676.c: Modify the include header file.
> * gcc.dg/tree-ssa/pr37508.c: Add the no-ssa-phiopt option to
> skip phi optimization.
> * gcc.dg/tree-ssa/pr88676.c: Rename to ...
> * gcc.dg/tree-ssa/pr88676-1.c: ... this new file.
> * gcc.dg/tree-ssa/pr88676-2.c: New testcase.
Please don't rename tests unless really necessary. Just keep pr88676.c
and add pr88676-2.c next to it.
> --- a/gcc/testsuite/gcc.dg/pr88676.c
> +++ b/gcc/testsuite/gcc.dg/pr88676.c
> @@ -2,7 +2,7 @@
> /* { dg-do run } */
> /* { dg-options "-O2" } */
>
> -#include "tree-ssa/pr88676.c"
> +#include "tree-ssa/pr88676-1.c"
>
> __attribute__((noipa)) void
> bar (int x, int y, int z)
Thus remove this hunk.
> rename from gcc/testsuite/gcc.dg/tree-ssa/pr88676.c
> rename to gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
And this one.
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-05 6:31 [PATCH] Fix a typo in two_value_replacement function Li Jia He
2019-05-05 9:48 ` Jakub Jelinek
@ 2019-05-06 11:19 ` Christophe Lyon
2019-05-06 11:35 ` Jakub Jelinek
2019-05-06 13:24 ` Li Jia He
1 sibling, 2 replies; 8+ messages in thread
From: Christophe Lyon @ 2019-05-06 11:19 UTC (permalink / raw)
To: Li Jia He
Cc: gcc Patches, Segher Boessenkool, wschmidt, Richard Biener, Jakub Jelinek
On Sun, 5 May 2019 at 08:31, Li Jia He <helijia@linux.ibm.com> wrote:
>
> Hi,
>
> GCC revision 267634 implemented two_value_replacement function.
> However, a typo occurred during the parameter check, which caused
> us to miss some optimizations.
>
> The intent of the code might be to check that the input parameters
> are const int and their difference is one. However, when I read
> the code, I found that it is wrong to detect whether an input data
> plus one is equal to itself. This could be a typo.
>
> The regression testing for the patch was done on GCC mainline on
>
> powerpc64le-unknown-linux-gnu (Power 9 LE)
>
> with no regressions. Is it OK for trunk and backport to gcc 9 ?
>
> Thanks,
> Lijia He
>
> gcc/ChangeLog
>
> 2019-05-05 Li Jia He <helijia@linux.ibm.com>
>
> * tree-ssa-phiopt.c (two_value_replacement):
> Fix a typo in parameter detection.
>
> gcc/testsuite/ChangeLog
>
> 2019-05-05 Li Jia He <helijia@linux.ibm.com>
>
> * gcc.dg/pr88676.c: Modify the include header file.
> * gcc.dg/tree-ssa/pr37508.c: Add the no-ssa-phiopt option to
> skip phi optimization.
> * gcc.dg/tree-ssa/pr88676.c: Rename to ...
> * gcc.dg/tree-ssa/pr88676-1.c: ... this new file.
> * gcc.dg/tree-ssa/pr88676-2.c: New testcase.
Hi,
This new testcase fails on arm and aarch64:
PASS: gcc.dg/tree-ssa/pr88676-2.c (test for excess errors)
UNRESOLVED: gcc.dg/tree-ssa/pr88676-2.c scan-tree-dump-not optimized " = PHI <"
because:
gcc.dg/tree-ssa/pr88676-2.c: dump file does not exist
Can you fix this?
Thanks,
Christophe
> ---
> gcc/testsuite/gcc.dg/pr88676.c | 2 +-
> gcc/testsuite/gcc.dg/tree-ssa/pr37508.c | 6 ++--
> .../tree-ssa/{pr88676.c => pr88676-1.c} | 0
> gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c | 30 +++++++++++++++++++
> gcc/tree-ssa-phiopt.c | 2 +-
> 5 files changed, 35 insertions(+), 5 deletions(-)
> rename gcc/testsuite/gcc.dg/tree-ssa/{pr88676.c => pr88676-1.c} (100%)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
>
> diff --git a/gcc/testsuite/gcc.dg/pr88676.c b/gcc/testsuite/gcc.dg/pr88676.c
> index b5fdd9342b8..719208083ae 100644
> --- a/gcc/testsuite/gcc.dg/pr88676.c
> +++ b/gcc/testsuite/gcc.dg/pr88676.c
> @@ -2,7 +2,7 @@
> /* { dg-do run } */
> /* { dg-options "-O2" } */
>
> -#include "tree-ssa/pr88676.c"
> +#include "tree-ssa/pr88676-1.c"
>
> __attribute__((noipa)) void
> bar (int x, int y, int z)
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c b/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
> index 2ba09afe481..a6def045de4 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-O2 -fno-tree-fre -fdump-tree-vrp1" } */
> +/* { dg-options "-O2 -fno-ssa-phiopt -fno-tree-fre -fdump-tree-vrp1" } */
>
> struct foo1 {
> int i:1;
> @@ -22,7 +22,7 @@ int test2 (struct foo2 *x)
> {
> if (x->i == 0)
> return 1;
> - else if (x->i == -1) /* This test is already folded to false by ccp1. */
> + else if (x->i == -1) /* This test is already optimized by ccp1 or phiopt1. */
> return 1;
> return 0;
> }
> @@ -31,7 +31,7 @@ int test3 (struct foo1 *x)
> {
> if (x->i == 0)
> return 1;
> - else if (x->i == 1) /* This test is already folded to false by fold. */
> + else if (x->i == 1) /* This test is already optimized by ccp1 or phiopt1. */
> return 1;
> return 0;
> }
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88676.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
> similarity index 100%
> rename from gcc/testsuite/gcc.dg/tree-ssa/pr88676.c
> rename to gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
> new file mode 100644
> index 00000000000..d377948e14d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
> @@ -0,0 +1,30 @@
> +/* PR tree-optimization/88676 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
> +
> +struct foo1 {
> + int i:1;
> +};
> +struct foo2 {
> + unsigned i:1;
> +};
> +
> +int test1 (struct foo1 *x)
> +{
> + if (x->i == 0)
> + return 1;
> + else if (x->i == 1)
> + return 1;
> + return 0;
> +}
> +
> +int test2 (struct foo2 *x)
> +{
> + if (x->i == 0)
> + return 1;
> + else if (x->i == -1)
> + return 1;
> + return 0;
> +}
> +
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 219791ea4ba..90674a2f3c4 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -602,7 +602,7 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> || TREE_CODE (arg1) != INTEGER_CST
> || (tree_int_cst_lt (arg0, arg1)
> ? wi::to_widest (arg0) + 1 != wi::to_widest (arg1)
> - : wi::to_widest (arg1) + 1 != wi::to_widest (arg1)))
> + : wi::to_widest (arg1) + 1 != wi::to_widest (arg0)))
> return false;
>
> if (!empty_block_p (middle_bb))
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-06 11:19 ` Christophe Lyon
@ 2019-05-06 11:35 ` Jakub Jelinek
2019-05-06 12:22 ` Li Jia He
2019-05-06 13:24 ` Li Jia He
1 sibling, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2019-05-06 11:35 UTC (permalink / raw)
To: Christophe Lyon, Li Jia He
Cc: gcc Patches, Segher Boessenkool, wschmidt, Richard Biener
On Mon, May 06, 2019 at 01:19:15PM +0200, Christophe Lyon wrote:
> > The regression testing for the patch was done on GCC mainline on
> >
> > powerpc64le-unknown-linux-gnu (Power 9 LE)
> >
> > with no regressions. Is it OK for trunk and backport to gcc 9 ?
While the posted patch had:
> > +/* PR tree-optimization/88676 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-optimized" } */
> > +/* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
the actually committed patch has:
/* PR tree-optimization/88676 */
/* { dg-do compile } */
/* { dg-options "-O2 -fdump-tree-phiopt1" } */
/* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
Dunno why this changed, if you want it in phiopt1, you need "phiopt1"
in scan-tree-dump-not as well, if you want optimized dump, you need
-fdump-tree-optimized instead.
>
> This new testcase fails on arm and aarch64:
> PASS: gcc.dg/tree-ssa/pr88676-2.c (test for excess errors)
> UNRESOLVED: gcc.dg/tree-ssa/pr88676-2.c scan-tree-dump-not optimized " = PHI <"
> because:
> gcc.dg/tree-ssa/pr88676-2.c: dump file does not exist
>
> Can you fix this?
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-06 11:35 ` Jakub Jelinek
@ 2019-05-06 12:22 ` Li Jia He
2019-05-06 12:27 ` Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Li Jia He @ 2019-05-06 12:22 UTC (permalink / raw)
To: Jakub Jelinek, Christophe Lyon
Cc: gcc Patches, Segher Boessenkool, wschmidt, Richard Biener
On 2019/5/6 7:35 PM, Jakub Jelinek wrote:
> On Mon, May 06, 2019 at 01:19:15PM +0200, Christophe Lyon wrote:
>>> The regression testing for the patch was done on GCC mainline on
>>>
>>> powerpc64le-unknown-linux-gnu (Power 9 LE)
>>>
>>> with no regressions. Is it OK for trunk and backport to gcc 9 ?
>
> While the posted patch had:
>>> +/* PR tree-optimization/88676 */
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>>> +/* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
> the actually committed patch has:
> /* PR tree-optimization/88676 */
> /* { dg-do compile } */
> /* { dg-options "-O2 -fdump-tree-phiopt1" } */
> /* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
>
> Dunno why this changed, if you want it in phiopt1, you need "phiopt1"
> in scan-tree-dump-not as well, if you want optimized dump, you need
> -fdump-tree-optimized instead.
When I test the code again for submiting the code, I found that this
change only affects the dump file generated by phiopt1,
so I rashly decided to change the test option to dump-tree-phiopt1. If
there is any code change in the future, I will send another patch.
Sorry for the error caused by this.
>>
>> This new testcase fails on arm and aarch64:
>> PASS: gcc.dg/tree-ssa/pr88676-2.c (test for excess errors)
>> UNRESOLVED: gcc.dg/tree-ssa/pr88676-2.c scan-tree-dump-not optimized " = PHI <"
>> because:
>> gcc.dg/tree-ssa/pr88676-2.c: dump file does not exist
>>
>> Can you fix this?
>
> Jakub
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-06 12:22 ` Li Jia He
@ 2019-05-06 12:27 ` Jakub Jelinek
2019-05-06 12:30 ` Li Jia He
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2019-05-06 12:27 UTC (permalink / raw)
To: Li Jia He
Cc: Christophe Lyon, gcc Patches, Segher Boessenkool, wschmidt,
Richard Biener
On Mon, May 06, 2019 at 08:22:41PM +0800, Li Jia He wrote:
> > Dunno why this changed, if you want it in phiopt1, you need "phiopt1"
> > in scan-tree-dump-not as well, if you want optimized dump, you need
> > -fdump-tree-optimized instead.
> When I test the code again for submiting the code, I found that this change
> only affects the dump file generated by phiopt1,
> so I rashly decided to change the test option to dump-tree-phiopt1. If there
> is any code change in the future, I will send another patch. Sorry for the
> error caused by this.
Even if you rush up such a change (happens sometimes), if it is testsuite
only you don't really need to bootstrap/regtest fully again, but retesting
the single testcase is needed. So in objdir/gcc
make check-gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} tree-ssa.exp=pr88676*'
in this case if the target is multilib, or
make check-gcc RUNTESTFLAGS='tree-ssa.exp=pr88676*'
otherwise.
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-06 12:27 ` Jakub Jelinek
@ 2019-05-06 12:30 ` Li Jia He
0 siblings, 0 replies; 8+ messages in thread
From: Li Jia He @ 2019-05-06 12:30 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Christophe Lyon, gcc Patches, Segher Boessenkool, wschmidt,
Richard Biener
On 2019/5/6 8:27 PM, Jakub Jelinek wrote:
> On Mon, May 06, 2019 at 08:22:41PM +0800, Li Jia He wrote:
>>> Dunno why this changed, if you want it in phiopt1, you need "phiopt1"
>>> in scan-tree-dump-not as well, if you want optimized dump, you need
>>> -fdump-tree-optimized instead.
>> When I test the code again for submiting the code, I found that this change
>> only affects the dump file generated by phiopt1,
>> so I rashly decided to change the test option to dump-tree-phiopt1. If there
>> is any code change in the future, I will send another patch. Sorry for the
>> error caused by this.
>
> Even if you rush up such a change (happens sometimes), if it is testsuite
> only you don't really need to bootstrap/regtest fully again, but retesting
> the single testcase is needed. So in objdir/gcc
> make check-gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} tree-ssa.exp=pr88676*'
> in this case if the target is multilib, or
> make check-gcc RUNTESTFLAGS='tree-ssa.exp=pr88676*'
> otherwise.
Thank you very much for this suggestion, I will be very careful in the
future.
>
> Jakub
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix a typo in two_value_replacement function
2019-05-06 11:19 ` Christophe Lyon
2019-05-06 11:35 ` Jakub Jelinek
@ 2019-05-06 13:24 ` Li Jia He
1 sibling, 0 replies; 8+ messages in thread
From: Li Jia He @ 2019-05-06 13:24 UTC (permalink / raw)
To: Christophe Lyon
Cc: gcc Patches, Segher Boessenkool, wschmidt, Richard Biener, Jakub Jelinek
On 2019/5/6 7:19 PM, Christophe Lyon wrote:
> On Sun, 5 May 2019 at 08:31, Li Jia He <helijia@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> GCC revision 267634 implemented two_value_replacement function.
>> However, a typo occurred during the parameter check, which caused
>> us to miss some optimizations.
>>
>> The intent of the code might be to check that the input parameters
>> are const int and their difference is one. However, when I read
>> the code, I found that it is wrong to detect whether an input data
>> plus one is equal to itself. This could be a typo.
>>
>> The regression testing for the patch was done on GCC mainline on
>>
>> powerpc64le-unknown-linux-gnu (Power 9 LE)
>>
>> with no regressions. Is it OK for trunk and backport to gcc 9 ?
>>
>> Thanks,
>> Lijia He
>>
>> gcc/ChangeLog
>>
>> 2019-05-05 Li Jia He <helijia@linux.ibm.com>
>>
>> * tree-ssa-phiopt.c (two_value_replacement):
>> Fix a typo in parameter detection.
>>
>> gcc/testsuite/ChangeLog
>>
>> 2019-05-05 Li Jia He <helijia@linux.ibm.com>
>>
>> * gcc.dg/pr88676.c: Modify the include header file.
>> * gcc.dg/tree-ssa/pr37508.c: Add the no-ssa-phiopt option to
>> skip phi optimization.
>> * gcc.dg/tree-ssa/pr88676.c: Rename to ...
>> * gcc.dg/tree-ssa/pr88676-1.c: ... this new file.
>> * gcc.dg/tree-ssa/pr88676-2.c: New testcase.
>
> Hi,
>
> This new testcase fails on arm and aarch64:
> PASS: gcc.dg/tree-ssa/pr88676-2.c (test for excess errors)
> UNRESOLVED: gcc.dg/tree-ssa/pr88676-2.c scan-tree-dump-not optimized " = PHI <"
> because:
> gcc.dg/tree-ssa/pr88676-2.c: dump file does not exist
>
> Can you fix this?
Sorry for this error, I have reverted it. Next time I will test it
well.
>
> Thanks,
>
> Christophe
>
>> ---
>> gcc/testsuite/gcc.dg/pr88676.c | 2 +-
>> gcc/testsuite/gcc.dg/tree-ssa/pr37508.c | 6 ++--
>> .../tree-ssa/{pr88676.c => pr88676-1.c} | 0
>> gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c | 30 +++++++++++++++++++
>> gcc/tree-ssa-phiopt.c | 2 +-
>> 5 files changed, 35 insertions(+), 5 deletions(-)
>> rename gcc/testsuite/gcc.dg/tree-ssa/{pr88676.c => pr88676-1.c} (100%)
>> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
>>
>> diff --git a/gcc/testsuite/gcc.dg/pr88676.c b/gcc/testsuite/gcc.dg/pr88676.c
>> index b5fdd9342b8..719208083ae 100644
>> --- a/gcc/testsuite/gcc.dg/pr88676.c
>> +++ b/gcc/testsuite/gcc.dg/pr88676.c
>> @@ -2,7 +2,7 @@
>> /* { dg-do run } */
>> /* { dg-options "-O2" } */
>>
>> -#include "tree-ssa/pr88676.c"
>> +#include "tree-ssa/pr88676-1.c"
>>
>> __attribute__((noipa)) void
>> bar (int x, int y, int z)
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c b/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
>> index 2ba09afe481..a6def045de4 100644
>> --- a/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr37508.c
>> @@ -1,5 +1,5 @@
>> /* { dg-do compile } */
>> -/* { dg-options "-O2 -fno-tree-fre -fdump-tree-vrp1" } */
>> +/* { dg-options "-O2 -fno-ssa-phiopt -fno-tree-fre -fdump-tree-vrp1" } */
>>
>> struct foo1 {
>> int i:1;
>> @@ -22,7 +22,7 @@ int test2 (struct foo2 *x)
>> {
>> if (x->i == 0)
>> return 1;
>> - else if (x->i == -1) /* This test is already folded to false by ccp1. */
>> + else if (x->i == -1) /* This test is already optimized by ccp1 or phiopt1. */
>> return 1;
>> return 0;
>> }
>> @@ -31,7 +31,7 @@ int test3 (struct foo1 *x)
>> {
>> if (x->i == 0)
>> return 1;
>> - else if (x->i == 1) /* This test is already folded to false by fold. */
>> + else if (x->i == 1) /* This test is already optimized by ccp1 or phiopt1. */
>> return 1;
>> return 0;
>> }
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88676.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
>> similarity index 100%
>> rename from gcc/testsuite/gcc.dg/tree-ssa/pr88676.c
>> rename to gcc/testsuite/gcc.dg/tree-ssa/pr88676-1.c
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
>> new file mode 100644
>> index 00000000000..d377948e14d
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr88676-2.c
>> @@ -0,0 +1,30 @@
>> +/* PR tree-optimization/88676 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>> +/* { dg-final { scan-tree-dump-not " = PHI <" "optimized" } } */
>> +
>> +struct foo1 {
>> + int i:1;
>> +};
>> +struct foo2 {
>> + unsigned i:1;
>> +};
>> +
>> +int test1 (struct foo1 *x)
>> +{
>> + if (x->i == 0)
>> + return 1;
>> + else if (x->i == 1)
>> + return 1;
>> + return 0;
>> +}
>> +
>> +int test2 (struct foo2 *x)
>> +{
>> + if (x->i == 0)
>> + return 1;
>> + else if (x->i == -1)
>> + return 1;
>> + return 0;
>> +}
>> +
>> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
>> index 219791ea4ba..90674a2f3c4 100644
>> --- a/gcc/tree-ssa-phiopt.c
>> +++ b/gcc/tree-ssa-phiopt.c
>> @@ -602,7 +602,7 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
>> || TREE_CODE (arg1) != INTEGER_CST
>> || (tree_int_cst_lt (arg0, arg1)
>> ? wi::to_widest (arg0) + 1 != wi::to_widest (arg1)
>> - : wi::to_widest (arg1) + 1 != wi::to_widest (arg1)))
>> + : wi::to_widest (arg1) + 1 != wi::to_widest (arg0)))
>> return false;
>>
>> if (!empty_block_p (middle_bb))
>> --
>> 2.17.1
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-06 13:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-05 6:31 [PATCH] Fix a typo in two_value_replacement function Li Jia He
2019-05-05 9:48 ` Jakub Jelinek
2019-05-06 11:19 ` Christophe Lyon
2019-05-06 11:35 ` Jakub Jelinek
2019-05-06 12:22 ` Li Jia He
2019-05-06 12:27 ` Jakub Jelinek
2019-05-06 12:30 ` Li Jia He
2019-05-06 13:24 ` Li Jia He
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).