| Message ID | 20140407162214.GA25019@maggie |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 7, 2014 at 6:22 PM, Andreas Krebbel <krebbel@linux.vnet.ibm.com> wrote: > On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: >> The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. > > They seem to require at least -O2 on x86 with that change. Ok to apply? Hmm, they passed before your change. Do you mean that this was by accident (and only because of the special return value)? If so then the patch is ok. Thanks, Richard. > diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-6.c b/gcc/testsuite/gcc.dg/builtin-bswap-6.c > index 6f0c782..f346c2f 100644 > --- a/gcc/testsuite/gcc.dg/builtin-bswap-6.c > +++ b/gcc/testsuite/gcc.dg/builtin-bswap-6.c > @@ -1,7 +1,7 @@ > /* { dg-do compile { target arm*-*-* alpha*-*-* i?86-*-* powerpc*-*-* rs6000-*-* x86_64-*-* s390*-*-* } } */ > /* { dg-require-effective-target stdint_types } */ > -/* { dg-options "-O -fdump-rtl-combine" } */ > -/* { dg-options "-O -fdump-rtl-combine -march=z900" { target s390-*-* } } */ > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > +/* { dg-options "-O2 -fdump-rtl-combine -march=z900" { target s390-*-* } } */ > > /* The test intentionally returns 1/2 instead of the obvious 0/1 to > prevent GCC from calculating the return value with arithmetic > diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-7.c b/gcc/testsuite/gcc.dg/builtin-bswap-7.c > index 0eecdd8..98529f2 100644 > --- a/gcc/testsuite/gcc.dg/builtin-bswap-7.c > +++ b/gcc/testsuite/gcc.dg/builtin-bswap-7.c > @@ -1,7 +1,7 @@ > /* { dg-do compile { target arm*-*-* alpha*-*-* ia64*-*-* x86_64-*-* s390x-*-* powerpc*-*-* rs6000-*-* } } */ > /* { dg-require-effective-target stdint_types } */ > /* { dg-require-effective-target lp64 } */ > -/* { dg-options "-O -fdump-rtl-combine" } */ > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > > /* The test intentionally returns 1/2 instead of the obvious 0/1 to > prevent GCC from calculating the return value with arithmetic >
On Mon, Apr 07, 2014 at 06:22:14PM +0200, Andreas Krebbel wrote: > On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: > > The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. > > They seem to require at least -O2 on x86 with that change. Ok to apply? The reason why the modified test now requires -O2 seems to be that during ce1 a conditional move is detected, but there is no DCE to clean stuff up before combine at -O1 and thus the bswapsi result pseudo has multiple uses. I guess your patch is ok for now, for 5.0 I'd say we should do it at the GIMPLE level, probably not in the optimize_bswap pass (because there it is guarded with bswap* insn patterns, while we can certainly optimize this for libcalls too), but say fold_builtins pass, when seeing __builtin_bswap* builtin that couldn't be folded, just check if the lhs has a single use that is a comparison with constant and if so, remove the swapping and adjust comparison. > diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-6.c b/gcc/testsuite/gcc.dg/builtin-bswap-6.c > index 6f0c782..f346c2f 100644 > --- a/gcc/testsuite/gcc.dg/builtin-bswap-6.c > +++ b/gcc/testsuite/gcc.dg/builtin-bswap-6.c > @@ -1,7 +1,7 @@ > /* { dg-do compile { target arm*-*-* alpha*-*-* i?86-*-* powerpc*-*-* rs6000-*-* x86_64-*-* s390*-*-* } } */ > /* { dg-require-effective-target stdint_types } */ > -/* { dg-options "-O -fdump-rtl-combine" } */ > -/* { dg-options "-O -fdump-rtl-combine -march=z900" { target s390-*-* } } */ > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > +/* { dg-options "-O2 -fdump-rtl-combine -march=z900" { target s390-*-* } } */ > > /* The test intentionally returns 1/2 instead of the obvious 0/1 to > prevent GCC from calculating the return value with arithmetic > diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-7.c b/gcc/testsuite/gcc.dg/builtin-bswap-7.c > index 0eecdd8..98529f2 100644 > --- a/gcc/testsuite/gcc.dg/builtin-bswap-7.c > +++ b/gcc/testsuite/gcc.dg/builtin-bswap-7.c > @@ -1,7 +1,7 @@ > /* { dg-do compile { target arm*-*-* alpha*-*-* ia64*-*-* x86_64-*-* s390x-*-* powerpc*-*-* rs6000-*-* } } */ > /* { dg-require-effective-target stdint_types } */ > /* { dg-require-effective-target lp64 } */ > -/* { dg-options "-O -fdump-rtl-combine" } */ > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > > /* The test intentionally returns 1/2 instead of the obvious 0/1 to > prevent GCC from calculating the return value with arithmetic Jakub
On Tue, Apr 8, 2014 at 10:31 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Apr 07, 2014 at 06:22:14PM +0200, Andreas Krebbel wrote: >> On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: >> > The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. >> >> They seem to require at least -O2 on x86 with that change. Ok to apply? > > The reason why the modified test now requires -O2 seems to be that during > ce1 a conditional move is detected, but there is no DCE to clean stuff > up before combine at -O1 and thus the bswapsi result pseudo has multiple > uses. > > I guess your patch is ok for now, for 5.0 I'd say we should do it at the > GIMPLE level, probably not in the optimize_bswap pass (because there it is > guarded with bswap* insn patterns, while we can certainly optimize this > for libcalls too), but say fold_builtins pass, when seeing > __builtin_bswap* builtin that couldn't be folded, just check if the lhs has > a single use that is a comparison with constant and if so, remove the > swapping and adjust comparison. Or just (match_and_simplify (eq (bswap @1) (bswap @2)) (eq @1 @2)) and similar patterns which should then catch it everywhere. Richard. >> diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-6.c b/gcc/testsuite/gcc.dg/builtin-bswap-6.c >> index 6f0c782..f346c2f 100644 >> --- a/gcc/testsuite/gcc.dg/builtin-bswap-6.c >> +++ b/gcc/testsuite/gcc.dg/builtin-bswap-6.c >> @@ -1,7 +1,7 @@ >> /* { dg-do compile { target arm*-*-* alpha*-*-* i?86-*-* powerpc*-*-* rs6000-*-* x86_64-*-* s390*-*-* } } */ >> /* { dg-require-effective-target stdint_types } */ >> -/* { dg-options "-O -fdump-rtl-combine" } */ >> -/* { dg-options "-O -fdump-rtl-combine -march=z900" { target s390-*-* } } */ >> +/* { dg-options "-O2 -fdump-rtl-combine" } */ >> +/* { dg-options "-O2 -fdump-rtl-combine -march=z900" { target s390-*-* } } */ >> >> /* The test intentionally returns 1/2 instead of the obvious 0/1 to >> prevent GCC from calculating the return value with arithmetic >> diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-7.c b/gcc/testsuite/gcc.dg/builtin-bswap-7.c >> index 0eecdd8..98529f2 100644 >> --- a/gcc/testsuite/gcc.dg/builtin-bswap-7.c >> +++ b/gcc/testsuite/gcc.dg/builtin-bswap-7.c >> @@ -1,7 +1,7 @@ >> /* { dg-do compile { target arm*-*-* alpha*-*-* ia64*-*-* x86_64-*-* s390x-*-* powerpc*-*-* rs6000-*-* } } */ >> /* { dg-require-effective-target stdint_types } */ >> /* { dg-require-effective-target lp64 } */ >> -/* { dg-options "-O -fdump-rtl-combine" } */ >> +/* { dg-options "-O2 -fdump-rtl-combine" } */ >> >> /* The test intentionally returns 1/2 instead of the obvious 0/1 to >> prevent GCC from calculating the return value with arithmetic > > Jakub
On Tue, Apr 08, 2014 at 10:26:30AM +0200, Richard Biener wrote: > On Mon, Apr 7, 2014 at 6:22 PM, Andreas Krebbel > <krebbel@linux.vnet.ibm.com> wrote: > > On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: > >> The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. > > > > They seem to require at least -O2 on x86 with that change. Ok to apply? > > Hmm, they passed before your change. Do you mean that this was > by accident (and only because of the special return value)? If so then > the patch is ok. The reason why it worked with the if ... return 1; else return 0; case is that in that case it has already been expanded as store flag insn and thus in that case ce1 pass didn't discover the conditional move there, thus no dead code waiting to be eliminated after ce1 and still present during combine pass. Another alternative for -O2 would be -O -fno-if-conversion I guess. OT, when touching the testcase, I'd say it would be better if you've converted it to single dg-options + /* { dg-additional-options "-march=z900" { target s390*-*-* } } */ > > --- a/gcc/testsuite/gcc.dg/builtin-bswap-6.c > > +++ b/gcc/testsuite/gcc.dg/builtin-bswap-6.c > > @@ -1,7 +1,7 @@ > > /* { dg-do compile { target arm*-*-* alpha*-*-* i?86-*-* powerpc*-*-* rs6000-*-* x86_64-*-* s390*-*-* } } */ > > /* { dg-require-effective-target stdint_types } */ > > -/* { dg-options "-O -fdump-rtl-combine" } */ > > -/* { dg-options "-O -fdump-rtl-combine -march=z900" { target s390-*-* } } */ > > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > > +/* { dg-options "-O2 -fdump-rtl-combine -march=z900" { target s390-*-* } } */ > > > > /* The test intentionally returns 1/2 instead of the obvious 0/1 to > > prevent GCC from calculating the return value with arithmetic > > diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-7.c b/gcc/testsuite/gcc.dg/builtin-bswap-7.c > > index 0eecdd8..98529f2 100644 > > --- a/gcc/testsuite/gcc.dg/builtin-bswap-7.c > > +++ b/gcc/testsuite/gcc.dg/builtin-bswap-7.c > > @@ -1,7 +1,7 @@ > > /* { dg-do compile { target arm*-*-* alpha*-*-* ia64*-*-* x86_64-*-* s390x-*-* powerpc*-*-* rs6000-*-* } } */ > > /* { dg-require-effective-target stdint_types } */ > > /* { dg-require-effective-target lp64 } */ > > -/* { dg-options "-O -fdump-rtl-combine" } */ > > +/* { dg-options "-O2 -fdump-rtl-combine" } */ > > > > /* The test intentionally returns 1/2 instead of the obvious 0/1 to > > prevent GCC from calculating the return value with arithmetic > > Jakub
On 04/08/2014 10:41 AM, Jakub Jelinek wrote: > On Tue, Apr 08, 2014 at 10:26:30AM +0200, Richard Biener wrote: >> On Mon, Apr 7, 2014 at 6:22 PM, Andreas Krebbel >> <krebbel@linux.vnet.ibm.com> wrote: >>> On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: >>>> The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. >>> >>> They seem to require at least -O2 on x86 with that change. Ok to apply? >> >> Hmm, they passed before your change. Do you mean that this was >> by accident (and only because of the special return value)? If so then >> the patch is ok. > > The reason why it worked with the if ... return 1; else return 0; case is > that in that case it has already been expanded as store flag insn and thus > in that case ce1 pass didn't discover the conditional move there, thus no > dead code waiting to be eliminated after ce1 and still present during > combine pass. > > Another alternative for -O2 would be -O -fno-if-conversion I guess. I could also revert the testcase changes and add -mbranch-cost=2 for s390?! > > OT, when touching the testcase, I'd say it would be better if you've > converted it to single dg-options + /* { dg-additional-options "-march=z900" { target s390*-*-* } } */ Ok. Bye, -Andreas- > >>> --- a/gcc/testsuite/gcc.dg/builtin-bswap-6.c >>> +++ b/gcc/testsuite/gcc.dg/builtin-bswap-6.c >>> @@ -1,7 +1,7 @@ >>> /* { dg-do compile { target arm*-*-* alpha*-*-* i?86-*-* powerpc*-*-* rs6000-*-* x86_64-*-* s390*-*-* } } */ >>> /* { dg-require-effective-target stdint_types } */ >>> -/* { dg-options "-O -fdump-rtl-combine" } */ >>> -/* { dg-options "-O -fdump-rtl-combine -march=z900" { target s390-*-* } } */ >>> +/* { dg-options "-O2 -fdump-rtl-combine" } */ >>> +/* { dg-options "-O2 -fdump-rtl-combine -march=z900" { target s390-*-* } } */ >>> >>> /* The test intentionally returns 1/2 instead of the obvious 0/1 to >>> prevent GCC from calculating the return value with arithmetic >>> diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-7.c b/gcc/testsuite/gcc.dg/builtin-bswap-7.c >>> index 0eecdd8..98529f2 100644 >>> --- a/gcc/testsuite/gcc.dg/builtin-bswap-7.c >>> +++ b/gcc/testsuite/gcc.dg/builtin-bswap-7.c >>> @@ -1,7 +1,7 @@ >>> /* { dg-do compile { target arm*-*-* alpha*-*-* ia64*-*-* x86_64-*-* s390x-*-* powerpc*-*-* rs6000-*-* } } */ >>> /* { dg-require-effective-target stdint_types } */ >>> /* { dg-require-effective-target lp64 } */ >>> -/* { dg-options "-O -fdump-rtl-combine" } */ >>> +/* { dg-options "-O2 -fdump-rtl-combine" } */ >>> >>> /* The test intentionally returns 1/2 instead of the obvious 0/1 to >>> prevent GCC from calculating the return value with arithmetic >>> > > Jakub >
On Tue, Apr 08, 2014 at 10:53:19AM +0200, Andreas Krebbel wrote: > On 04/08/2014 10:41 AM, Jakub Jelinek wrote: > > On Tue, Apr 08, 2014 at 10:26:30AM +0200, Richard Biener wrote: > >> On Mon, Apr 7, 2014 at 6:22 PM, Andreas Krebbel > >> <krebbel@linux.vnet.ibm.com> wrote: > >>> On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: > >>>> The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. > >>> > >>> They seem to require at least -O2 on x86 with that change. Ok to apply? > >> > >> Hmm, they passed before your change. Do you mean that this was > >> by accident (and only because of the special return value)? If so then > >> the patch is ok. > > > > The reason why it worked with the if ... return 1; else return 0; case is > > that in that case it has already been expanded as store flag insn and thus > > in that case ce1 pass didn't discover the conditional move there, thus no > > dead code waiting to be eliminated after ce1 and still present during > > combine pass. > > > > Another alternative for -O2 would be -O -fno-if-conversion I guess. > > I could also revert the testcase changes and add -mbranch-cost=2 for s390?! That doesn't seem to work (at least for me with cross-compiler to s390x). > > > > OT, when touching the testcase, I'd say it would be better if you've > > converted it to single dg-options + /* { dg-additional-options "-march=z900" { target s390*-*-* } } */ > > Ok. Thanks. Jakub
On 04/08/2014 11:12 AM, Jakub Jelinek wrote: > On Tue, Apr 08, 2014 at 10:53:19AM +0200, Andreas Krebbel wrote: >> On 04/08/2014 10:41 AM, Jakub Jelinek wrote: >>> On Tue, Apr 08, 2014 at 10:26:30AM +0200, Richard Biener wrote: >>>> On Mon, Apr 7, 2014 at 6:22 PM, Andreas Krebbel >>>> <krebbel@linux.vnet.ibm.com> wrote: >>>>> On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: >>>>>> The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. >>>>> >>>>> They seem to require at least -O2 on x86 with that change. Ok to apply? >>>> >>>> Hmm, they passed before your change. Do you mean that this was >>>> by accident (and only because of the special return value)? If so then >>>> the patch is ok. >>> >>> The reason why it worked with the if ... return 1; else return 0; case is >>> that in that case it has already been expanded as store flag insn and thus >>> in that case ce1 pass didn't discover the conditional move there, thus no >>> dead code waiting to be eliminated after ce1 and still present during >>> combine pass. >>> >>> Another alternative for -O2 would be -O -fno-if-conversion I guess. >> >> I could also revert the testcase changes and add -mbranch-cost=2 for s390?! > > That doesn't seem to work (at least for me with cross-compiler to s390x). Seems to be -mbranch-cost=0 this time. > >>> >>> OT, when touching the testcase, I'd say it would be better if you've >>> converted it to single dg-options + /* { dg-additional-options "-march=z900" { target s390*-*-* } } */ >> >> Ok. > > Thanks. > > Jakub >
On Tue, Apr 08, 2014 at 11:21:56AM +0200, Andreas Krebbel wrote: > On 04/08/2014 11:12 AM, Jakub Jelinek wrote: > > On Tue, Apr 08, 2014 at 10:53:19AM +0200, Andreas Krebbel wrote: > >> On 04/08/2014 10:41 AM, Jakub Jelinek wrote: > >>> On Tue, Apr 08, 2014 at 10:26:30AM +0200, Richard Biener wrote: > >>>> On Mon, Apr 7, 2014 at 6:22 PM, Andreas Krebbel > >>>> <krebbel@linux.vnet.ibm.com> wrote: > >>>>> On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: > >>>>>> The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. > >>>>> > >>>>> They seem to require at least -O2 on x86 with that change. Ok to apply? > >>>> > >>>> Hmm, they passed before your change. Do you mean that this was > >>>> by accident (and only because of the special return value)? If so then > >>>> the patch is ok. > >>> > >>> The reason why it worked with the if ... return 1; else return 0; case is > >>> that in that case it has already been expanded as store flag insn and thus > >>> in that case ce1 pass didn't discover the conditional move there, thus no > >>> dead code waiting to be eliminated after ce1 and still present during > >>> combine pass. > >>> > >>> Another alternative for -O2 would be -O -fno-if-conversion I guess. > >> > >> I could also revert the testcase changes and add -mbranch-cost=2 for s390?! > > > > That doesn't seem to work (at least for me with cross-compiler to s390x). > > Seems to be -mbranch-cost=0 this time. Indeed, that works too. Thus, please commit any of these variants, if you go for /* { dg-additional-options "-mbranch-cost=0" { target s390*-*-* } } */ plus reverting your earlier changes, you can also consider addition of two new tests that would contain the return {1,2} and have -O2. Jakub
diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-6.c b/gcc/testsuite/gcc.dg/builtin-bswap-6.c index 6f0c782..f346c2f 100644 --- a/gcc/testsuite/gcc.dg/builtin-bswap-6.c +++ b/gcc/testsuite/gcc.dg/builtin-bswap-6.c @@ -1,7 +1,7 @@ /* { dg-do compile { target arm*-*-* alpha*-*-* i?86-*-* powerpc*-*-* rs6000-*-* x86_64-*-* s390*-*-* } } */ /* { dg-require-effective-target stdint_types } */ -/* { dg-options "-O -fdump-rtl-combine" } */ -/* { dg-options "-O -fdump-rtl-combine -march=z900" { target s390-*-* } } */ +/* { dg-options "-O2 -fdump-rtl-combine" } */ +/* { dg-options "-O2 -fdump-rtl-combine -march=z900" { target s390-*-* } } */ /* The test intentionally returns 1/2 instead of the obvious 0/1 to prevent GCC from calculating the return value with arithmetic diff --git a/gcc/testsuite/gcc.dg/builtin-bswap-7.c b/gcc/testsuite/gcc.dg/builtin-bswap-7.c index 0eecdd8..98529f2 100644 --- a/gcc/testsuite/gcc.dg/builtin-bswap-7.c +++ b/gcc/testsuite/gcc.dg/builtin-bswap-7.c @@ -1,7 +1,7 @@ /* { dg-do compile { target arm*-*-* alpha*-*-* ia64*-*-* x86_64-*-* s390x-*-* powerpc*-*-* rs6000-*-* } } */ /* { dg-require-effective-target stdint_types } */ /* { dg-require-effective-target lp64 } */ -/* { dg-options "-O -fdump-rtl-combine" } */ +/* { dg-options "-O2 -fdump-rtl-combine" } */ /* The test intentionally returns 1/2 instead of the obvious 0/1 to prevent GCC from calculating the return value with arithmetic
On Mon, Apr 07, 2014 at 04:19:06PM +0200, Richard Biener wrote: > The adjusted testcases now fail on x86_64/i?86 at least. See PR60776. They seem to require at least -O2 on x86 with that change. Ok to apply?