From: Christophe Lyon <christophe.lyon@linaro.org>
To: Li Jia He <helijia@linux.ibm.com>
Cc: gcc Patches <gcc-patches@gcc.gnu.org>,
Segher Boessenkool <segher@kernel.crashing.org>,
wschmidt@linux.ibm.com, Richard Biener <rguenther@suse.de>,
Jakub Jelinek <jakub@redhat.com>
Subject: Re: [PATCH] Fix a typo in two_value_replacement function
Date: Mon, 06 May 2019 11:19:00 -0000 [thread overview]
Message-ID: <CAKdteOZ45DpJJwvMk1hdbxk4CeySjTcmbZxvjqv7Z_pNEDj6kQ@mail.gmail.com> (raw)
In-Reply-To: <1557037872-47239-1-git-send-email-helijia@linux.ibm.com>
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
>
next prev parent reply other threads:[~2019-05-06 11:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-05 6:31 Li Jia He
2019-05-05 9:48 ` Jakub Jelinek
2019-05-06 11:19 ` Christophe Lyon [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKdteOZ45DpJJwvMk1hdbxk4CeySjTcmbZxvjqv7Z_pNEDj6kQ@mail.gmail.com \
--to=christophe.lyon@linaro.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=helijia@linux.ibm.com \
--cc=jakub@redhat.com \
--cc=rguenther@suse.de \
--cc=segher@kernel.crashing.org \
--cc=wschmidt@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).