| Message ID | 20101214094155.GD27214@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/14/10 02:41, Jakub Jelinek wrote: > Hi! > > In 4.4 gimple_references_memory_p checked a bit in the stmt, but starting > with 4.5 that inline just looks at vuses. Unfortunately, at -O0 there > are no vuses and thus is_replaceable_p at -O0 -ftree-ter happily moves > memory loads around. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk? > > 2010-12-14 Jakub Jelinek<jakub@redhat.com> > > PR tree-optimization/43655 > * tree-ssa-ter.c (is_replaceable_p): Don't use > gimple_references_memory_p for -O0, instead check for load > by looking at rhs. > > * g++.dg/opt/pr43655.C: New test. I'm a little confused as to why TER is running when we're not optimizing. There's no doubt more bugs of this nature lying around if we allow specific optimizers to be turned on at -O0. If we're going to allow optimizers to run at -O0, wouldn't it be better to fix gimple_references_memory_p rather than have to twiddle each of its callers to DTRT at -O0. At least one of the other users of gimple_references_memory_p seems to be prone to a similar bug (tree-tailcall.c). Jeff
On Tue, Dec 14, 2010 at 2:56 PM, Jeff Law <law@redhat.com> wrote: > On 12/14/10 02:41, Jakub Jelinek wrote: >> >> Hi! >> >> In 4.4 gimple_references_memory_p checked a bit in the stmt, but starting >> with 4.5 that inline just looks at vuses. Unfortunately, at -O0 there >> are no vuses and thus is_replaceable_p at -O0 -ftree-ter happily moves >> memory loads around. >> >> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok >> for >> trunk? >> >> 2010-12-14 Jakub Jelinek<jakub@redhat.com> >> >> PR tree-optimization/43655 >> * tree-ssa-ter.c (is_replaceable_p): Don't use >> gimple_references_memory_p for -O0, instead check for load >> by looking at rhs. >> >> * g++.dg/opt/pr43655.C: New test. > > I'm a little confused as to why TER is running when we're not optimizing. > There's no doubt more bugs of this nature lying around if we allow specific > optimizers to be turned on at -O0. > > If we're going to allow optimizers to run at -O0, wouldn't it be better to > fix gimple_references_memory_p rather than have to twiddle each of its > callers to DTRT at -O0. At least one of the other users of > gimple_references_memory_p seems to be prone to a similar bug > (tree-tailcall.c). I originally wanted to get rid of gimple_references_memory_p. For the specific test is_gimple_val (rhs) is probably enough (given that we don't TER aggregate copies). Richard. > Jeff > > > >
On Wed, Dec 15, 2010 at 03:26:52AM +0100, Richard Guenther wrote: > On Tue, Dec 14, 2010 at 2:56 PM, Jeff Law <law@redhat.com> wrote: > > I'm a little confused as to why TER is running when we're not optimizing. > > There's no doubt more bugs of this nature lying around if we allow specific > > optimizers to be turned on at -O0. I think the reason we allow it is unit testing, plus, eventually we want some (limited) TER at -O0 for inline asm operands to get rid of the regressions tree-SSA and tuplification caused. > > If we're going to allow optimizers to run at -O0, wouldn't it be better to > > fix gimple_references_memory_p rather than have to twiddle each of its > > callers to DTRT at -O0. At least one of the other users of > > gimple_references_memory_p seems to be prone to a similar bug > > (tree-tailcall.c). > > I originally wanted to get rid of gimple_references_memory_p. For the specific > test is_gimple_val (rhs) is probably enough (given that we don't TER aggregate > copies). You mean if (!optimized && gimple_assign_single_p (stmt) && !is_gimple_val (gimple_assign_rhs1 (stmt))) return false; ? As for the other uses of gimple_references_memory_p, I guess most if not all are never called for -O0, because neither gate_all_optimizations nor gate_all_early_optimizations allows most of the late tree passes (and early optimization passes) at all at -O0, no matter if they are enabled or not. That includes ccp, dom, loop opts, tail call/recursion which use that function. And also pre, ealias and alias which use TODO_rebuild_alias. So I think tree-ter was the only -O0 user of that function. Jakub
On Wed, Dec 15, 2010 at 9:34 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Dec 15, 2010 at 03:26:52AM +0100, Richard Guenther wrote: >> On Tue, Dec 14, 2010 at 2:56 PM, Jeff Law <law@redhat.com> wrote: >> > I'm a little confused as to why TER is running when we're not optimizing. >> > There's no doubt more bugs of this nature lying around if we allow specific >> > optimizers to be turned on at -O0. > > I think the reason we allow it is unit testing, plus, eventually we want > some (limited) TER at -O0 for inline asm operands to get rid of the > regressions tree-SSA and tuplification caused. > >> > If we're going to allow optimizers to run at -O0, wouldn't it be better to >> > fix gimple_references_memory_p rather than have to twiddle each of its >> > callers to DTRT at -O0. At least one of the other users of >> > gimple_references_memory_p seems to be prone to a similar bug >> > (tree-tailcall.c). >> >> I originally wanted to get rid of gimple_references_memory_p. For the specific >> test is_gimple_val (rhs) is probably enough (given that we don't TER aggregate >> copies). > > You mean > if (!optimized > && gimple_assign_single_p (stmt) > && !is_gimple_val (gimple_assign_rhs1 (stmt))) > return false; > ? Yes. > As for the other uses of gimple_references_memory_p, I guess most if not all > are never called for -O0, because neither gate_all_optimizations nor > gate_all_early_optimizations allows most of the late tree passes (and early > optimization passes) at all at -O0, no matter if they are enabled or not. > That includes ccp, dom, loop opts, tail call/recursion which use that > function. And also pre, ealias and alias which use TODO_rebuild_alias. > So I think tree-ter was the only -O0 user of that function. Yep. I think I converted most callers to gimple_reference_memory_p to test gimple_vuse instead during alias-improvements merge but then halfway decided that gimple_references_memory_p would be easier to grok. Well ;) The patch is ok with the above change. Thanks, Richard. > Jakub >
--- gcc/tree-ssa-ter.c.jj 2010-11-01 09:07:23.000000000 +0100 +++ gcc/tree-ssa-ter.c 2010-12-13 11:16:03.000000000 +0100 @@ -416,8 +416,14 @@ is_replaceable_p (gimple stmt) return false; /* Without alias info we can't move around loads. */ - if (gimple_references_memory_p (stmt) && !optimize) - return false; + if (!optimize && gimple_assign_single_p (stmt)) + { + tree rhs = gimple_assign_rhs1 (stmt); + if (TREE_CODE (rhs) == VAR_DECL + || TREE_CODE (rhs) == MEM_REF + || handled_component_p (rhs)) + return false; + } /* Float expressions must go through memory if float-store is on. */ if (flag_float_store --- gcc/testsuite/g++.dg/opt/pr43655.C.jj 2010-12-13 11:22:38.000000000 +0100 +++ gcc/testsuite/g++.dg/opt/pr43655.C 2010-12-13 11:21:58.000000000 +0100 @@ -0,0 +1,34 @@ +// PR tree-optimization/43655 +// { dg-do run } +// { dg-options "-O0 -ftree-ter" } + +extern "C" void abort (); + +struct C +{ + C (int i) : val(i) { } + C (const C& c) : val(c.val) { } + ~C (void) { val = 999; } + C& operator = (const C& c) { val = c.val; return *this; } + C& inc (int i) { val += i; return *this; } + int val; +}; + +C +f () +{ + return C (3); +} + +C +f (int i) +{ + return f ().inc (i); +} + +int +main () +{ + if (f (2).val != 5) + abort (); +}