Message ID | 52664C2B.60400@mentor.com |
---|---|
State | New |
Headers | show |
On 10/22/13 03:58, Tom de Vries wrote: > Richard, > > This patch adds a missing check for gimple_vdef in stmt_local_def for the > tail-merge pass. > > Bootstrapped and reg-tested on x86_64. > > OK for trunk, gcc-4_8-branch? > > Thanks, > - Tom > > 2013-10-22 Tom de Vries <tom@codesourcery.com> > > PR tree-optimization/58805 > * tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check. > > * gcc.dg/pr58805.c: New test. Doesn't this test belong in an architecture specific directory? Under what conditions can a statement have a VDEF but not be considered as having a side effect by gimple_has_side_effects? It almost seems to me that gimple_has_side_effects may need updating. jeff
On Tue, 22 Oct 2013, Jeff Law wrote: > On 10/22/13 03:58, Tom de Vries wrote: > > Richard, > > > > This patch adds a missing check for gimple_vdef in stmt_local_def for the > > tail-merge pass. > > > > Bootstrapped and reg-tested on x86_64. > > > > OK for trunk, gcc-4_8-branch? > > > > Thanks, > > - Tom > > > > 2013-10-22 Tom de Vries <tom@codesourcery.com> > > > > PR tree-optimization/58805 > > * tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check. > > > > * gcc.dg/pr58805.c: New test. > Doesn't this test belong in an architecture specific directory? > > Under what conditions can a statement have a VDEF but not be considered as > having a side effect by gimple_has_side_effects? > > It almost seems to me that gimple_has_side_effects may need updating. You seem to misunderstand "side-effect", for example *p = 1; has !gimple_has_side_effects but it has a VDEF. Likewise *p = const_call_returing_aggregate (); has !gimple_has_side_effects but it has a VDEF. side-effect is an effect that is not explicitely represented in the gimple stmt you look at. Richard.
On Tue, 22 Oct 2013, Tom de Vries wrote: > Richard, > > This patch adds a missing check for gimple_vdef in stmt_local_def for the > tail-merge pass. > > Bootstrapped and reg-tested on x86_64. > > OK for trunk, gcc-4_8-branch? Ok. Thanks, Richard.
On 22/10/13 20:50, Jeff Law wrote: > On 10/22/13 03:58, Tom de Vries wrote: >> Richard, >> >> This patch adds a missing check for gimple_vdef in stmt_local_def for the >> tail-merge pass. >> >> Bootstrapped and reg-tested on x86_64. >> >> OK for trunk, gcc-4_8-branch? >> >> Thanks, >> - Tom >> >> 2013-10-22 Tom de Vries <tom@codesourcery.com> >> >> PR tree-optimization/58805 >> * tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check. >> >> * gcc.dg/pr58805.c: New test. > Doesn't this test belong in an architecture specific directory? > Jeff, The test-case has i386 assembly inside the asm string, but since the test-case only compiles, the assembly string is never used. I've made the string empty to make that clear. AFAIU the only requirement for this test-case is that the constraint matches the operand. I'm not sure whether 'unsigned long' always matches 'r'. I've changed this into 'void *' and 'p', which I think should always be true. Committed as below. Thanks, - Tom > jeff > +++ b/gcc/testsuite/gcc.dg/pr58805.c @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -ftree-tail-merge -fdump-tree-pre" } */ + +/* Type that matches the 'p' constraint. */ +#define TYPE void * + +static inline +void bar (TYPE *r) +{ + TYPE t; + __asm__ ("" : "=&p" (t), "=p" (*r)); +} + +void +foo (int n, TYPE *x, TYPE *y) +{ + if (n == 0) + bar (x); + else + bar (y); +} + +/* { dg-final { scan-tree-dump-times "__asm__" 2 "pre"} } */ +/* { dg-final { cleanup-tree-dump "pre" } } */
On 10/23/13 10:02, Tom de Vries wrote: > On 22/10/13 20:50, Jeff Law wrote: >> On 10/22/13 03:58, Tom de Vries wrote: >>> Richard, >>> >>> This patch adds a missing check for gimple_vdef in stmt_local_def for the >>> tail-merge pass. >>> >>> Bootstrapped and reg-tested on x86_64. >>> >>> OK for trunk, gcc-4_8-branch? >>> >>> Thanks, >>> - Tom >>> >>> 2013-10-22 Tom de Vries <tom@codesourcery.com> >>> >>> PR tree-optimization/58805 >>> * tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check. >>> >>> * gcc.dg/pr58805.c: New test. >> Doesn't this test belong in an architecture specific directory? >> > > Jeff, > > The test-case has i386 assembly inside the asm string, but since the test-case > only compiles, the assembly string is never used. I've made the string empty to > make that clear. Thanks. > > AFAIU the only requirement for this test-case is that the constraint matches the > operand. I'm not sure whether 'unsigned long' always matches 'r'. I've changed > this into 'void *' and 'p', which I think should always be true. > > Committed as below. Excellent. Thanks again. jeff
On 10/23/13 02:16, Richard Biener wrote: > On Tue, 22 Oct 2013, Jeff Law wrote: > >> On 10/22/13 03:58, Tom de Vries wrote: >>> Richard, >>> >>> This patch adds a missing check for gimple_vdef in stmt_local_def for the >>> tail-merge pass. >>> >>> Bootstrapped and reg-tested on x86_64. >>> >>> OK for trunk, gcc-4_8-branch? >>> >>> Thanks, >>> - Tom >>> >>> 2013-10-22 Tom de Vries <tom@codesourcery.com> >>> >>> PR tree-optimization/58805 >>> * tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check. >>> >>> * gcc.dg/pr58805.c: New test. >> Doesn't this test belong in an architecture specific directory? >> >> Under what conditions can a statement have a VDEF but not be considered as >> having a side effect by gimple_has_side_effects? >> >> It almost seems to me that gimple_has_side_effects may need updating. > > You seem to misunderstand "side-effect", for example > > *p = 1; > > has !gimple_has_side_effects but it has a VDEF. Likewise > > *p = const_call_returing_aggregate (); > > has !gimple_has_side_effects but it has a VDEF. side-effect is > an effect that is not explicitely represented in the gimple stmt > you look at. So side effects as in implicit... That makes sense. Thanks for the clarification. jeff
diff --git a/gcc/testsuite/gcc.dg/pr58805.c b/gcc/testsuite/gcc.dg/pr58805.c new file mode 100644 index 0000000..6e6eba5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr58805.c @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -ftree-tail-merge -fdump-tree-pre" } */ + +static inline void bar(unsigned long *r) +{ + unsigned long t; + __asm__ ( + "movq $42, %[t]\n\t" + "movq %[t], %[r]\n\t" + : [t] "=&r" (t), [r] "=r" (*r) + ); +} + +void foo(int n, unsigned long *x, unsigned long *y) +{ + if (n == 0) + bar(x); + else + bar(y); +} + +/* { dg-final { scan-tree-dump-times "__asm__" 2 "pre"} } */ +/* { dg-final { cleanup-tree-dump "pre" } } */ diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c index 9094935..0090da6 100644 --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -300,7 +300,8 @@ stmt_local_def (gimple stmt) tree val; def_operand_p def_p; - if (gimple_has_side_effects (stmt)) + if (gimple_has_side_effects (stmt) + || gimple_vdef (stmt) != NULL_TREE) return false; def_p = SINGLE_SSA_DEF_OPERAND (stmt, SSA_OP_DEF);