Bug 51879 - Missed tail merging with non-const/pure calls
Summary: Missed tail merging with non-const/pure calls
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.7.0
: P3 enhancement
Target Milestone: ---
Assignee: Tom de Vries
URL:
Keywords: missed-optimization
Depends on: 51877
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-17 11:11 UTC by Jakub Jelinek
Modified: 2012-07-06 11:31 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-01-18 00:00:00


Attachments
Tentative patch (1.01 KB, patch)
2012-01-23 14:10 UTC, Tom de Vries
Details | Diff
Updated tentative patch (1.74 KB, patch)
2012-01-25 09:46 UTC, Tom de Vries
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2012-01-17 11:11:41 UTC
We don't tail merge currently:
int bar (int);
void baz (int);

void
foo (int y)
{
  int a;
  if (y == 6)
    a = bar (7);
  else
    a = bar (7);
  baz (a);
}

(both before the PR51877 fix and after it), because both SSA_NAMEs on the lhs of the calls aren't valueized the same by SCCVN.
Similarly for:
  if (y == 6)
    a = bar (7) + 6;
  else
    a = bar (7) + 6;
or
  if (y)
    baz (bar (7) + 6);
  else
    baz (bar (7) + 6);
Comment 1 Tom de Vries 2012-01-18 05:41:34 UTC
This works for the 3 examples:
...
Index: tree-ssa-sccvn.c
===================================================================
--- tree-ssa-sccvn.c (revision 182098)
+++ tree-ssa-sccvn.c (working copy)
@@ -3360,8 +3360,7 @@ visit_use (tree use)
 	  /* ???  We should handle stores from calls.  */
 	  else if (TREE_CODE (lhs) == SSA_NAME)
 	    {
-	      if (!gimple_call_internal_p (stmt)
-		  && gimple_call_flags (stmt) & (ECF_PURE | ECF_CONST))
+	      if (!gimple_call_internal_p (stmt))
 		changed = visit_reference_op_call (lhs, stmt);
 	      else
 		changed = defs_to_varying (stmt);
...

I'll test this to see what breaks.
Comment 2 Jakub Jelinek 2012-01-18 10:36:39 UTC
I'm afraid a lot would break.  It really depends on what you use VN for and on what code.
If you have:
  D.12345_1 = bar (7);
  D.12346_2 = bar (7);
and bar isn't const/pure call, then if VN equivalences D.12345_1 and D.12346_2, it is wrong.  Of course if you have:
<bb7>:
  D.12345_1 = bar (7);
  goto bb9;
<bb8>:
  D.12346_2 = bar (7);
<bb9>:
  D.12347_3 = PHI <D.12345_1(7), D.12346_2(8)>
(this case), you could VN them the same.
Comment 3 Richard Biener 2012-01-18 10:45:01 UTC
(In reply to comment #2)
> I'm afraid a lot would break.  It really depends on what you use VN for and on
> what code.
> If you have:
>   D.12345_1 = bar (7);
>   D.12346_2 = bar (7);
> and bar isn't const/pure call, then if VN equivalences D.12345_1 and D.12346_2,
> it is wrong.  Of course if you have:
> <bb7>:
>   D.12345_1 = bar (7);
>   goto bb9;
> <bb8>:
>   D.12346_2 = bar (7);
> <bb9>:
>   D.12347_3 = PHI <D.12345_1(7), D.12346_2(8)>
> (this case), you could VN them the same.

Yes, but not for

  D.12345_1 = bar (7);
  D.12346_2 = bar (7);

so you can't really value-number the calls the same.  As we are working
on SSA SCCs and not on a CFG (and thus do not do predicated value-numbering)
that ability is useless.
Comment 4 Richard Biener 2012-01-18 10:46:00 UTC
Confirmed btw.
Comment 5 Tom de Vries 2012-01-23 14:10:38 UTC
Created attachment 26430 [details]
Tentative patch

2012-01-23  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/51879
	tree-ssa-sccvn.c (visit_reference_op_call): Handle gimple_vdef.
	(visit_use): Handle non-pure/const calls using visit_reference_op_call.

	gcc.dg/pr51879.c: New test.
	gcc.dg/pr51879-2.c: Same.
	gcc.dg/pr51879-3.c: Same.
	gcc.dg/pr51879-4.c: Same.
Comment 6 Tom de Vries 2012-01-24 10:12:38 UTC
Submitted: http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01178.html
Comment 7 Tom de Vries 2012-01-25 09:46:52 UTC
Created attachment 26454 [details]
Updated tentative patch

This patch was bootstrapped and reg-tested on x86_64, and fails only in 3 tests
- gcc.c-torture/compile/20030224-1.c
- gcc.c-torture/execute/20020412-1.c
- gcc.dg/lto/20090706-1_0.c
on ICE from PR51990.

2012-01-25  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/51879
	* tree-ssa-sccvn.h (struct vn_reference_s): Add vdef field.
	* tree-ssa-sccvn.c (visit_reference_op_call): Handle gimple_vdef.
	Handle case that lhs is NULL_TREE.
	(visit_use): Handle non-pure/const calls and calls without result using
	visit_reference_op_call.

	gcc.dg/pr51879.c: New test.
	gcc.dg/pr51879-2.c: Same.
	gcc.dg/pr51879-3.c: Same.
	gcc.dg/pr51879-4.c: Same.
	gcc.dg/pr51879-6.c: Same.
Comment 8 Tom de Vries 2012-02-01 11:20:31 UTC
submitted patch: http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01513.html
Comment 9 Tom de Vries 2012-04-27 06:12:55 UTC
Author: vries
Date: Fri Apr 27 06:12:49 2012
New Revision: 186894

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186894
Log:
2012-04-27  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/51879
	* tree-ssa-sccvn.h (struct vn_reference_s): Add result_vdef field.
	* tree-ssa-sccvn.c (mark_use_processed): New function, factored out
	of ...
	(defs_to_varying): ... here.  Don't set use_processed.
	(visit_reference_op_call): Handle gimple_vdef.
	Handle case that lhs is NULL_TREE.
	(visit_use): Use mark_use_processed.  Handle calls with side-effect
	using visit_reference_op_call.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa-sccvn.c
    trunk/gcc/tree-ssa-sccvn.h
Comment 10 Tom de Vries 2012-04-27 06:28:55 UTC
Author: vries
Date: Fri Apr 27 06:28:49 2012
New Revision: 186895

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186895
Log:
2012-04-27  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/51879
	* gcc.dg/pr51879.c: New test.
	* gcc.dg/pr51879-2.c: Same.
	* gcc.dg/pr51879-3.c: Same.
	* gcc.dg/pr51879-4.c: Same.
	* gcc.dg/pr51879-6.c: Same.

Added:
    trunk/gcc/testsuite/gcc.dg/pr51879-2.c
    trunk/gcc/testsuite/gcc.dg/pr51879-3.c
    trunk/gcc/testsuite/gcc.dg/pr51879-4.c
    trunk/gcc/testsuite/gcc.dg/pr51879-6.c
    trunk/gcc/testsuite/gcc.dg/pr51879.c
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 11 Tom de Vries 2012-04-27 06:35:30 UTC
All listed examples fixed in r186894. 

Todo: follow-up with fix for:
...
struct S { int i; };
extern struct S foo (void);
extern int foo2 (void);
struct S s;
int bar (int c) {
  int r;
  if (c)
    {
      s = foo ();
      r = foo2 ();
    }
  else
    {
      s = foo ();
      r = foo2 ();
    }
  return r;
}
...
Comment 12 Tom de Vries 2012-07-06 11:22:10 UTC
Author: vries
Date: Fri Jul  6 11:22:06 2012
New Revision: 189323

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189323
Log:
2012-07-06  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/51879
	* tree-ssa-sccvn.c (copy_reference_ops_from_call)
	(visit_reference_op_call): Handle case that lhs is not an SSA_NAME.
	(visit_use): Also call visit_reference_op_call for calls with a vdef.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa-sccvn.c
Comment 13 Tom de Vries 2012-07-06 11:22:15 UTC
Author: vries
Date: Fri Jul  6 11:22:12 2012
New Revision: 189324

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189324
Log:
2012-07-06  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/51879
	* gcc.dg/pr51879-16.c: New test.
	* gcc.dg/pr51879-17.c: Same.

Added:
    trunk/gcc/testsuite/gcc.dg/pr51879-16.c
    trunk/gcc/testsuite/gcc.dg/pr51879-17.c
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 14 Tom de Vries 2012-07-06 11:31:33 UTC
follow-up patch and test-cases checked in, closing bug.