Message ID | 1557037872-47239-1-git-send-email-helijia@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | Fix a typo in two_value_replacement function | expand |
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
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 >
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
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 >
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
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 >
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 >> >
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))