| Message ID | 1576823645-73942-1-git-send-email-guojiufu@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | enable -fweb and -frename-registers at -O3 for rs6000 | expand |
Hi! On Fri, Dec 20, 2019 at 02:34:05PM +0800, Jiufu Guo wrote: > Previously, limited unrolling was enabled at O2 for powerpc in r278034. At that > time, -fweb and -frename-registers were not enabled together with -funroll-loops > even for -O3. After that, we notice there are some performance degradation on > SPEC2006fp which caused by without web and rnreg. And 2017 was fine on all tests. Annoying :-( > This patch enable -fweb > and -frename-registers for -O3 to align original behavior before r278034. Okay. Hopefully we can find a way to determine in what circumstances web and rnreg help instead of hurt, but until then, the old behaviour is certainly the safe choice. > --- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c > +++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c > @@ -2,6 +2,7 @@ > /* Originator: Andrew Church <gcczilla@achurch.org> */ > /* { dg-do run } */ > /* { dg-require-effective-target untyped_assembly } */ > +/* { dg-additional-options "-fno-rename-registers" { target { powerpc*-*-* } } } */ What is this for? What happens without it? The rs6000/ parts are okay for trunk. Thanks! Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > Hi! > > On Fri, Dec 20, 2019 at 02:34:05PM +0800, Jiufu Guo wrote: >> Previously, limited unrolling was enabled at O2 for powerpc in r278034. At that >> time, -fweb and -frename-registers were not enabled together with -funroll-loops >> even for -O3. After that, we notice there are some performance degradation on >> SPEC2006fp which caused by without web and rnreg. > > And 2017 was fine on all tests. Annoying :-( > >> This patch enable -fweb >> and -frename-registers for -O3 to align original behavior before r278034. > > Okay. Hopefully we can find a way to determine in what circumstances web > and rnreg help instead of hurt, but until then, the old behaviour is > certainly the safe choice. > >> --- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c >> +++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c >> @@ -2,6 +2,7 @@ >> /* Originator: Andrew Church <gcczilla@achurch.org> */ >> /* { dg-do run } */ >> /* { dg-require-effective-target untyped_assembly } */ >> +/* { dg-additional-options "-fno-rename-registers" { target { powerpc*-*-* } } } */ > > What is this for? What happens without it? The reason of this fail is: -frename-registers does not work well with __builtin_return/__builtin_apply which need to save and restore registers which could not be renamed incorrectly. When this case runs with -O3, with this patch, -frename-registers is enabled. Originally, -frename-registers is enabled with -funroll-loops instead pure -O3. This change cause this case fail at -O3. > > The rs6000/ parts are okay for trunk. Thanks! > > > Segher
Jiufu Guo <guojiufu@linux.ibm.com> writes: > Segher Boessenkool <segher@kernel.crashing.org> writes: > >> Hi! >> [...] >>> --- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c >>> +++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c >>> @@ -2,6 +2,7 @@ >>> /* Originator: Andrew Church <gcczilla@achurch.org> */ >>> /* { dg-do run } */ >>> /* { dg-require-effective-target untyped_assembly } */ >>> +/* { dg-additional-options "-fno-rename-registers" { target { powerpc*-*-* } } } */ >> >> What is this for? What happens without it? > The reason of this fail is: -frename-registers does not work well with > __builtin_return/__builtin_apply which need to save and restore > registers which could not be renamed incorrectly. For this issue, I opened a bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93047. Thanks, Jiufu. > > When this case runs with -O3, with this patch, -frename-registers is > enabled. Originally, -frename-registers is enabled with -funroll-loops > instead pure -O3. This change cause this case fail at -O3. > >> >> The rs6000/ parts are okay for trunk. Thanks! >> >> >> Segher
diff --git a/gcc/common/config/rs6000/rs6000-common.c b/gcc/common/config/rs6000/rs6000-common.c index eb0328d..75a7caf 100644 --- a/gcc/common/config/rs6000/rs6000-common.c +++ b/gcc/common/config/rs6000/rs6000-common.c @@ -38,10 +38,9 @@ static const struct default_options rs6000_option_optimization_table[] = loops at -O2 and above by default. */ { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 }, { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL, 1 }, - /* -fweb and -frename-registers are useless in general for rs6000, - turn them off. */ - { OPT_LEVELS_ALL, OPT_fweb, NULL, 0 }, - { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 }, + /* -fweb and -frename-registers are useful only above -O3 for rs6000. */ + { OPT_LEVELS_3_PLUS, OPT_fweb, NULL, 1 }, + { OPT_LEVELS_3_PLUS, OPT_frename_registers, NULL, 1 }, /* Double growth factor to counter reduced min jump length. */ { OPT_LEVELS_ALL, OPT__param_max_grow_copy_bb_insns_, NULL, 16 }, diff --git a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c index ec4fd8a..4e342c0 100644 --- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c +++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c @@ -2,6 +2,7 @@ /* Originator: Andrew Church <gcczilla@achurch.org> */ /* { dg-do run } */ /* { dg-require-effective-target untyped_assembly } */ +/* { dg-additional-options "-fno-rename-registers" { target { powerpc*-*-* } } } */ /* This used to fail on SPARC because the (undefined) return value of 'bar' was overwriting that of 'foo'. */