Patchwork Fix -ftree-ter -O0 (PR tree-optimization/43655)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 14, 2010, 9:41 a.m.
Message ID <20101214094155.GD27214@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/75477/
State New
Headers show

Comments

Jakub Jelinek - Dec. 14, 2010, 9:41 a.m.
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.


	Jakub
Jeff Law - Dec. 14, 2010, 1:56 p.m.
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
Richard Guenther - Dec. 15, 2010, 2:26 a.m.
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
>
>
>
>
Jakub Jelinek - Dec. 15, 2010, 8:34 a.m.
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
Richard Guenther - Dec. 16, 2010, 11:17 a.m.
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
>

Patch

--- 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 ();
+}