From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126858 invoked by alias); 6 May 2019 11:19:31 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 126729 invoked by uid 89); 6 May 2019 11:19:31 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-15.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Envelope-From:sk:christo X-HELO: mail-vs1-f66.google.com Received: from mail-vs1-f66.google.com (HELO mail-vs1-f66.google.com) (209.85.217.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 06 May 2019 11:19:29 +0000 Received: by mail-vs1-f66.google.com with SMTP id g127so7877027vsd.6 for ; Mon, 06 May 2019 04:19:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=cBwJ6BIml47IVDXM128TpD6NlzGhgBdFMEgpm/SrNvY=; b=jcyVw0SOzuY8wHIwX4QgcfEnnA60N8qQoKKCXae4XQWemw/5ukaiPj9MbEclg+g9B3 K9UBCJToJ6YWzw+Kanfxc3ZTz4msnWK7/1bmDPDrMzcVTLTWdbJU+yDAqR6I+VEORk3C ed7280X7nHJ+8Gi1AgT6CgQzuft1PBZzII408JH7JKoqfaT2RjYSjhvaSsMsRytj87VH NAIQkjN1TT+z01ojd3GlCJ9Z/E/5x+UgEREZ4J2U8SVLSJFzvY558O66MM7iyghfoIPd Audz4otkb8haHF0Q0dZkfPac4OgDib4b24ubiWHjn6dF7Ib+/niTcC+LvPEzxGVekkfM WaeA== MIME-Version: 1.0 References: <1557037872-47239-1-git-send-email-helijia@linux.ibm.com> In-Reply-To: <1557037872-47239-1-git-send-email-helijia@linux.ibm.com> From: Christophe Lyon Date: Mon, 06 May 2019 11:19:00 -0000 Message-ID: Subject: Re: [PATCH] Fix a typo in two_value_replacement function To: Li Jia He Cc: gcc Patches , Segher Boessenkool , wschmidt@linux.ibm.com, Richard Biener , Jakub Jelinek Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00196.txt.bz2 On Sun, 5 May 2019 at 08:31, Li Jia He 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 > > * tree-ssa-phiopt.c (two_value_replacement): > Fix a typo in parameter detection. > > gcc/testsuite/ChangeLog > > 2019-05-05 Li Jia He > > * 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 >