| Message ID | 53F6098F.9000506@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Aug 21, 2014, at 8:00 AM, Sandra Loosemore <sandra@codesourcery.com> wrote:
> tests that assume some non-default branch costs in the back end
Thanks.
One comment, could you put in /* non default branch cost */ above the three where that is true. Even better would be to refactor those into something for target supports. Though, really the best course of action would be for someone to list the targets where the non default condition holds true, and make those pay the price, not the universal complement.
On 08/21/2014 11:36 AM, Mike Stump wrote: > On Aug 21, 2014, at 8:00 AM, Sandra Loosemore > <sandra@codesourcery.com> wrote: >> tests that assume some non-default branch costs in the back end > > Thanks. > > One comment, could you put in /* non default branch cost */ above the > three where that is true. The three what? :-S > Even better would be to refactor those > into something for target supports. Though, really the best course > of action would be for someone to list the targets where the non > default condition holds true, and make those pay the price, not the > universal complement. I definitely agree about this. Perhaps the tree-ssa maintainers can take care of this, or at least clarify what target(s) this is intended to work on rather than having to experimentally determine which ones don't implement these optimizations, or propose some way to test for the support rather than having to list targets explicitly. -Sandra
On Aug 21, 2014, at 10:59 AM, Sandra Loosemore <sandra@codesourcery.com> wrote: > On 08/21/2014 11:36 AM, Mike Stump wrote: >> On Aug 21, 2014, at 8:00 AM, Sandra Loosemore >> <sandra@codesourcery.com> wrote: >>> tests that assume some non-default branch costs in the back end >> >> Thanks. >> >> One comment, could you put in /* non default branch cost */ above the >> three where that is true. > > The three what? :-S Sorry, I meant 4… Your patch has four instances of this change: -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ see your patch for them. Can you change the patch effectively to: -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ +/* non default branch cost */ +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ instead? The comment serves as documentation as to what all the listed targets have in common. A person doing a new port, can then read the comment, and say I am non-default branch cost, so add me, or alternatively, I am the default, this failure is a bug I need to investigate and fix. > I definitely agree about this. Perhaps the tree-ssa maintainers can take care of this, I added law as he added one of the test cases. :-) > or at least clarify what target(s) this is intended to work on rather than having to experimentally determine which ones don't implement these optimizations, or propose some way to test for the support rather than having to list targets explicitly. I’d be fine with the entire list changing to the one target that the test case was put in for. That would be better than how it is now.
On 08/21/2014 03:50 PM, Mike Stump wrote: > > Sorry, I meant 4… Your patch has four instances of this change: > > -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ > +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > > see your patch for them. Can you change the patch effectively to: > > -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ > +/* non default branch cost */ > +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > > instead? The comment serves as documentation as to what all the listed targets > have in common. A person doing a new port, can then read the comment, and say > I am non-default branch cost, so add me, or alternatively, I am the default, > this failure is a bug I need to investigate and fix. Hmmm, I think this is backwards. At least nios2 does not override BRANCH_COST. The default value (in defaults.h) is 1; when I was investigating these tree-ssa failures I saw that the test being used to enable the optimizations is >= 2. It's also quite possible for back ends to override BRANCH_COST but in a way that still causes the tests to fail, at least for some multilibs or processor options. I'd really like the maintainers of these tree-ssa tests to figure out what target they're supposed to work for or come up with a suitable test for feature support, rather than me trying to guess the failure mode for all these other back ends I can't test. -Sandra
On Aug 21, 2014, at 5:06 PM, Sandra Loosemore <sandra@codesourcery.com> wrote:
> I'd really like the maintainers of these tree-ssa tests to figure out what target they're supposed to work for or come up with a suitable test for feature support, rather than me trying to guess the failure mode for all these other back ends I can't test.
I know the feeling… :-)
On Thu, 21 Aug 2014, Mike Stump wrote: > On Aug 21, 2014, at 10:59 AM, Sandra Loosemore <sandra@codesourcery.com> wrote: > > On 08/21/2014 11:36 AM, Mike Stump wrote: > >> On Aug 21, 2014, at 8:00 AM, Sandra Loosemore > >> <sandra@codesourcery.com> wrote: > >>> tests that assume some non-default branch costs in the back end > >> > >> Thanks. > >> > >> One comment, could you put in /* non default branch cost */ above the > >> three where that is true. > > > > The three what? :-S > > Sorry, I meant 4? Your patch has four instances of this change: > > -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ > +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > > see your patch for them. Can you change the patch effectively to: > > -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ > +/* non default branch cost */ > +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > > instead? The comment serves as documentation as to what all > the listed targets have in common. A person doing a new port, > can then read the comment, and say I am non-default branch cost, > so add me, or alternatively, I am the default, this failure is a > bug I need to investigate and fix. It's the other way round re "listed targets". (CRIS and MMIX have the default branch cost, ditto m68k and moxie, didn't check the others.) brgds, H-P
On Aug 22, 2014, at 3:48 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > >> +/* non default branch cost */ >> +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > It's the other way round re "listed targets". (CRIS and MMIX > have the default branch cost, ditto m68k and moxie, didn't check > the others.) Right. I had ! on the brain. Do you like non-default branch cost, or would it be better to describe it some other way?
On 08/23/2014 10:26 AM, Mike Stump wrote: > On Aug 22, 2014, at 3:48 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: >> >>> +/* non default branch cost */ >>> +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > >> It's the other way round re "listed targets". (CRIS and MMIX >> have the default branch cost, ditto m68k and moxie, didn't check >> the others.) > > Right. I had ! on the brain. Do you like non-default branch cost, or would it > be better to describe it some other way? If we're going to go through the trouble of adding a comment here, we might as well make it something less terse and more explicit, like: These tests fail unless the target backend overrides BRANCH_COST to return a value >= 2. -Sandra
On Aug 23, 2014, at 9:37 AM, Sandra Loosemore <sandra@codesourcery.com> wrote: > If we're going to go through the trouble of adding a comment here, we might as well make it something less terse and more explicit, like: > > These tests fail unless the target backend overrides BRANCH_COST to return a value >= 2. Sounds good to me. :-)
On Sat, 23 Aug 2014, Sandra Loosemore wrote: > On 08/23/2014 10:26 AM, Mike Stump wrote: > > On Aug 22, 2014, at 3:48 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > > > > > > > +/* non default branch cost */ > > > > +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* > > > > v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* > > > > mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ > > > > > It's the other way round re "listed targets". (CRIS and MMIX > > > have the default branch cost, ditto m68k and moxie, didn't check > > > the others.) > > > > Right. I had ! on the brain. Do you like non-default branch cost, or would > > it > > be better to describe it some other way? > > If we're going to go through the trouble of adding a comment here, we might as > well make it something less terse and more explicit, like: > > These tests fail unless the target backend overrides BRANCH_COST to return a > value >= 2. Agreed! The target list line is getting longish, maybe put it in an effective-target proc while it's being edited anyway. brgds, H-P
Index: gcc/testsuite/lib/target-supports.exp =================================================================== --- gcc/testsuite/lib/target-supports.exp (revision 214241) +++ gcc/testsuite/lib/target-supports.exp (working copy) @@ -5970,6 +5970,7 @@ proc check_effective_target_logical_op_s || [istarget mmix-*-*] || [istarget s390*-*-*] || [istarget powerpc*-*-*] + || [istarget nios2*-*-*] || [check_effective_target_arm_cortex_m] } { return 1 } Index: gcc/testsuite/gcc.dg/tree-ssa/reassoc-33.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/reassoc-33.c (revision 214241) +++ gcc/testsuite/gcc.dg/tree-ssa/reassoc-33.c (working copy) @@ -1,4 +1,4 @@ -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ /* { dg-options "-O2 -fno-inline -fdump-tree-reassoc1-details" } */ /* { dg-additional-options "-mbranch-cost=2" { target mips*-*-* avr-*-* s390*-*-* i?86-*-* x86_64-*-* } } */ Index: gcc/testsuite/gcc.dg/tree-ssa/reassoc-34.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/reassoc-34.c (revision 214241) +++ gcc/testsuite/gcc.dg/tree-ssa/reassoc-34.c (working copy) @@ -1,4 +1,4 @@ -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ /* { dg-options "-O2 -fno-inline -fdump-tree-reassoc1-details" } */ /* { dg-additional-options "-mbranch-cost=2" { target mips*-*-* avr-*-* s390*-*-* i?86-*-* x86_64-*-* } } */ Index: gcc/testsuite/gcc.dg/tree-ssa/reassoc-35.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/reassoc-35.c (revision 214241) +++ gcc/testsuite/gcc.dg/tree-ssa/reassoc-35.c (working copy) @@ -1,4 +1,4 @@ -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ /* { dg-options "-O2 -fno-inline -fdump-tree-reassoc1-details" } */ /* { dg-additional-options "-mbranch-cost=2" { target mips*-*-* avr-*-* s390*-*-* i?86-*-* x86_64-*-* } } */ Index: gcc/testsuite/gcc.dg/tree-ssa/reassoc-36.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/reassoc-36.c (revision 214241) +++ gcc/testsuite/gcc.dg/tree-ssa/reassoc-36.c (working copy) @@ -1,4 +1,4 @@ -/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-*"} } } */ +/* { dg-do run { target { ! "m68k*-*-* mmix*-*-* mep*-*-* bfin*-*-* v850*-*-* picochip*-*-* moxie*-*-* cris*-*-* m32c*-*-* fr30*-*-* mcore*-*-* powerpc*-*-* xtensa*-*-* hppa*-*-* nios2*-*-*"} } } */ /* { dg-options "-O2 -fno-inline -fdump-tree-reassoc1-details" } */ /* { dg-additional-options "-mbranch-cost=2" { target mips*-*-* avr-*-* s390*-*-* i?86-*-* x86_64-*-* } } */ Index: gcc/testsuite/gcc.dg/tree-ssa/interposition.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/interposition.c (revision 214241) +++ gcc/testsuite/gcc.dg/tree-ssa/interposition.c (working copy) @@ -1,4 +1,5 @@ /* { dg-do compile } */ +/* { dg-require-effective-target fpic } */ /* { dg-options "-O1 -fno-semantic-interposition -fdump-tree-optimized -fPIC" } */ int t(void) { Index: gcc/testsuite/gcc.dg/lto/pr61526_0.c =================================================================== --- gcc/testsuite/gcc.dg/lto/pr61526_0.c (revision 214241) +++ gcc/testsuite/gcc.dg/lto/pr61526_0.c (working copy) @@ -1,3 +1,4 @@ +/* { dg-require-effective-target fpic } */ /* { dg-lto-do link } */ /* { dg-lto-options { { -fPIC -flto -flto-partition=1to1 } } } */ /* { dg-extra-ld-options { -shared } } */