diff mbox

PR65217.c, improve canonicalization of implied copy from equality comparison

Message ID 553F0792.8080404@redhat.com
State New
Headers show

Commit Message

Jeff Law April 28, 2015, 4:07 a.m. UTC 
Richi recently changed tree-ssa-dom.c::record_equality to use 
tree_swap_operands_p to canonicalize the implied copy we record for 
equality comparisons.  This is a good thing.

However, there is a case where tree_swap_operands_p gives us operands in 
an undesirable order for this routine.  Specifically when both operands 
are SSA_NAMEs, but just one has a single use (in the equality test).  In 
that case, we want the single use SSA_NAME on the LHS of the recorded copy.

That, in effect, prevents DOM from destroying the single use nature of 
that SSA_NAME.  And if we're ultimately able to remove the comparison, 
the statements that fed the single use SSA_NAME can be removed as 
they'll be dead.

Given this issue is so closely tied to DOM, Richi and I agreed to fix 
this in DOM rather than in tree_swap_operands_p.  Had we tried to 
address this in tree_swap_operands_p, we'll end up regressing elsewhere, 
which is clearly not good.

Anyway, bootstrapped and regression tested on x86_64-linux-gnu.  Fixes 
the recently XFAILed 65217 test.  Installed on the trunk.

Jeff
commit a787980e8012128b408ca8b716028e71d1b21373
Author: root <root@localhost.localdomain>
Date:   Mon Apr 27 21:58:52 2015 -0600

    	PR tree-optimization/65217
    	* tree-ssa-dom.c (record_equality): Given two SSA_NAMEs, if just one
    	of them has a single use, make sure it is the LHS of the implied
    	copy.
    
            PR tree-optimization/65217
    	* gcc.target/i386/pr65217.c: Remove XFAIL.

Comments

Marek Polacek April 28, 2015, 7:15 a.m. UTC | #1 
On Mon, Apr 27, 2015 at 10:07:46PM -0600, Jeff Law wrote:

Thanks for the nice summary in the first part of the mail.  Two typos:

> +  /* Most of the time tree_swap_operands_p does what we want.  But there's

Shouldn't that be "there are"?

> +     cases where we we know one operand is better for copy propagation than

Redundant "we".

	Marek
Jeff Law April 28, 2015, 11:18 p.m. UTC | #2 
On 04/28/2015 01:15 AM, Marek Polacek wrote:
> On Mon, Apr 27, 2015 at 10:07:46PM -0600, Jeff Law wrote:
>
> Thanks for the nice summary in the first part of the mail.  Two typos:
>
>> +  /* Most of the time tree_swap_operands_p does what we want.  But there's
>
> Shouldn't that be "there are"?
>
>> +     cases where we we know one operand is better for copy propagation than
>
> Redundant "we".
Thanks.  Fixed in the obvious way.

jeff
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index fff0015..1a82f17 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@ 
+2015-04-27  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/65217
+	* tree-ssa-dom.c (record_equality): Given two SSA_NAMEs, if just one
+	of them has a single use, make sure it is the LHS of the implied
+	copy.
+
 2015-04-28  Alan Modra  <amodra@gmail.com>
 
 	PR target/65810
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 67bdd69..0fc2384 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2015-04-27  Jeff Law <law@redhat.com>
+
+        PR tree-optimization/65217
+	* gcc.target/i386/pr65217.c: Remove XFAIL.
+
 2015-04-27  Andre Vehreschild  <vehre@gmx.de>
 
 	PR fortran/60322
diff --git a/gcc/testsuite/gcc.target/i386/pr65217.c b/gcc/testsuite/gcc.target/i386/pr65217.c
index 3f495b2..d5c9be5 100644
--- a/gcc/testsuite/gcc.target/i386/pr65217.c
+++ b/gcc/testsuite/gcc.target/i386/pr65217.c
@@ -1,7 +1,7 @@ 
 /* { dg-do compile } */
 /* { dg-options "-O" } */
-/* { dg-final { scan-assembler-not "negl" { xfail *-*-* } } } */
-/* { dg-final { scan-assembler-not "andl" { xfail *-*-* } } } */
+/* { dg-final { scan-assembler-not "negl" } } */
+/* { dg-final { scan-assembler-not "andl" } } */
 
 int 
 test(int n)
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index a67b4e4..c7d427b 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -1762,6 +1762,20 @@  record_equality (tree x, tree y)
   if (tree_swap_operands_p (x, y, false))
     std::swap (x, y);
 
+  /* Most of the time tree_swap_operands_p does what we want.  But there's
+     cases where we we know one operand is better for copy propagation than
+     the other.  Given no other code cares about ordering of equality
+     comparison operators for that purpose, we just handle the special cases
+     here.  */
+  if (TREE_CODE (x) == SSA_NAME && TREE_CODE (y) == SSA_NAME)
+    {
+      /* If one operand is a single use operand, then make it
+	 X.  This will preserve its single use properly and if this
+	 conditional is eliminated, the computation of X can be
+	 eliminated as well.  */
+      if (has_single_use (y) && ! has_single_use (x))
+	std::swap (x, y);
+    }
   if (TREE_CODE (x) == SSA_NAME)
     prev_x = SSA_NAME_VALUE (x);
   if (TREE_CODE (y) == SSA_NAME)