public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [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).