Re: PR tree-optimization/54985

2012-10-26 Thread Jeff Law

On 10/23/2012 03:43 PM, Jakub Jelinek wrote:

On Tue, Oct 23, 2012 at 03:34:46PM -0600, Jeff Law wrote:

I think it should be backported to 4.7, perhaps with a few days delay after the
trunk commit.

Do we even have debug statements after control flow statements?


They shouldn't be there, so if you just give up the same way for gsi_stmt
NULL as well as non-control stmt, it shouldn't make a difference.
So last_stmt might be just shorter to type and more commonly used, nothing
more.

Done.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.


* tree-ssa-threadedge.c (cond_arg_set_in_bb): Use last stmt.

diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 6249e2f..ddaa7d1 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -584,7 +584,7 @@ cond_arg_set_in_bb (edge e, basic_block bb)
 {
   ssa_op_iter iter;
   use_operand_p use_p;
-  gimple last = gsi_stmt (gsi_last_bb (e-dest));
+  gimple last = last_stmt (e-dest);
 
   /* E-dest does not have to end with a control transferring
  instruction.  This can occurr when we try to extend a jump


Re: PR tree-optimization/54985

2012-10-24 Thread Jeff Law

On 10/23/2012 03:43 PM, Jakub Jelinek wrote:

On Tue, Oct 23, 2012 at 03:34:46PM -0600, Jeff Law wrote:

I think it should be backported to 4.7, perhaps with a few days delay after the
trunk commit.

Do we even have debug statements after control flow statements?


They shouldn't be there, so if you just give up the same way for gsi_stmt
NULL as well as non-control stmt, it shouldn't make a difference.
So last_stmt might be just shorter to type and more commonly used, nothing
more.
From looking at the existing code and last_stmt; for the cases we care 
about, they ought to be equivalent.


Using last_stmt seems marginally clearer.  I'll go ahead and make that 
change after the usual bootstrap  test cycle.


jeff




PR tree-optimization/54985

2012-10-23 Thread Jeff Law


When we try to thread across a back edge in the CFG we have a check to 
ensure that we don't use temporary equivalences which are invalidated by 
traversal of the back edge to simplify the final conditional.


About a year ago I added code to pick up secondary threading 
opportunities after an initial jump threading was successful.  However, 
I failed to account for equivalences which were invalidated by the back 
edge traversal in those new cases.


This patch extracts the check into its own function, then calls it from 
the 3 locations where it's necessary.


Bootstrapped and regression tested on x86_64-unknown-linux-gnu. 
Installed onto the trunk.


Oh how I want to rewrite all this code


Jeff

ps.  First time commiting using git svn, hopefully I did all the steps 
correctly.


* tree-ssa-threadedge.c (cond_arg_set_in_bb): New function extracted
from thread_across_edge.
(thread_across_edge): Use it in all cases where we might thread
across a back edge.

* gcc.c-torture/execute/pr54985.c: New test.

diff --git a/gcc/testsuite/gcc.c-torture/execute/pr54985.c 
b/gcc/testsuite/gcc.c-torture/execute/pr54985.c
new file mode 100644
index 000..678c9f4
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr54985.c
@@ -0,0 +1,36 @@
+
+typedef struct st {
+int a;
+} ST;
+
+int __attribute__((noinline,noclone))
+foo(ST *s, int c)
+{
+  int first = 1;
+  int count = c;
+  ST *item = s;
+  int a = s-a;
+  int x;
+
+  while (count--)
+{
+  x = item-a;
+  if (first)
+first = 0;
+  else if (x = a)
+return 1;
+  a = x;
+  item++;
+}
+  return 0;
+}
+
+extern void abort (void);
+
+int main ()
+{
+  ST _1[2] = {{2}, {1}};
+  if (foo(_1, 2) != 0)
+abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 105e3ab..491aa9f 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -572,6 +572,44 @@ simplify_control_stmt_condition (edge e,
   return cached_lhs;
 }
 
+/* Return TRUE if the statement at the end of e-dest depends on
+   the output of any statement in BB.   Otherwise return FALSE.
+
+   This is used when we are threading a backedge and need to ensure
+   that temporary equivalences from BB do not affect the condition
+   in e-dest.  */
+
+static bool
+cond_arg_set_in_bb (edge e, basic_block bb, int n)
+{
+  ssa_op_iter iter;
+  use_operand_p use_p;
+  gimple last = gsi_stmt (gsi_last_bb (e-dest));
+
+  /* E-dest does not have to end with a control transferring
+ instruction.  This can occurr when we try to extend a jump
+ threading opportunity deeper into the CFG.  In that case
+ it is safe for this check to return false.  */
+  if (!last)
+return false;
+
+  if (gimple_code (last) != GIMPLE_COND
+   gimple_code (last) != GIMPLE_GOTO
+   gimple_code (last) != GIMPLE_SWITCH)
+return false;
+
+  FOR_EACH_SSA_USE_OPERAND (use_p, last, iter, SSA_OP_USE | SSA_OP_VUSE)
+{
+  tree use = USE_FROM_PTR (use_p);
+
+  if (TREE_CODE (use) == SSA_NAME
+  gimple_code (SSA_NAME_DEF_STMT (use)) != GIMPLE_PHI
+  gimple_bb (SSA_NAME_DEF_STMT (use)) == bb)
+   return true;
+}
+  return false;
+}
+
 /* TAKEN_EDGE represents the an edge taken as a result of jump threading.
See if we can thread around TAKEN_EDGE-dest as well.  If so, return
the edge out of TAKEN_EDGE-dest that we can statically compute will be
@@ -705,19 +743,8 @@ thread_across_edge (gimple dummy_cond,
  safe to thread this edge.  */
   if (e-flags  EDGE_DFS_BACK)
 {
-  ssa_op_iter iter;
-  use_operand_p use_p;
-  gimple last = gsi_stmt (gsi_last_bb (e-dest));
-
-  FOR_EACH_SSA_USE_OPERAND (use_p, last, iter, SSA_OP_USE | SSA_OP_VUSE)
-   {
- tree use = USE_FROM_PTR (use_p);
-
-  if (TREE_CODE (use) == SSA_NAME
-  gimple_code (SSA_NAME_DEF_STMT (use)) != GIMPLE_PHI
-  gimple_bb (SSA_NAME_DEF_STMT (use)) == e-dest)
-   goto fail;
-   }
+  if (cond_arg_set_in_bb (e, e-dest, 1))
+   goto fail;
 }
 
   stmt_count = 0;
@@ -758,7 +785,9 @@ thread_across_edge (gimple dummy_cond,
 address.  If DEST is not null, then see if we can thread
 through it as well, this helps capture secondary effects
 of threading without having to re-run DOM or VRP.  */
- if (dest)
+ if (dest
+  ((e-flags  EDGE_DFS_BACK) == 0
+ || ! cond_arg_set_in_bb (taken_edge, e-dest, 2)))
{
  /* We don't want to thread back to a block we have already
 visited.  This may be overly conservative.  */
@@ -816,11 +845,16 @@ thread_across_edge (gimple dummy_cond,
e3 = taken_edge;
do
  {
-   e2 = thread_around_empty_block (e3,
-   dummy_cond,
-   

Re: PR tree-optimization/54985

2012-10-23 Thread Jakub Jelinek
On Tue, Oct 23, 2012 at 02:35:24PM -0600, Jeff Law wrote:
 +/* Return TRUE if the statement at the end of e-dest depends on
 +   the output of any statement in BB.   Otherwise return FALSE.
 +
 +   This is used when we are threading a backedge and need to ensure
 +   that temporary equivalences from BB do not affect the condition
 +   in e-dest.  */
 +
 +static bool
 +cond_arg_set_in_bb (edge e, basic_block bb, int n)
 +{
 +  ssa_op_iter iter;
 +  use_operand_p use_p;
 +  gimple last = gsi_stmt (gsi_last_bb (e-dest));

Use gimple last = last_stmt (e-dest); instead?  That way any possible
debug stmts are ignored.

Jakub


Re: PR tree-optimization/54985

2012-10-23 Thread Jeff Law

On 10/23/2012 02:50 PM, Jakub Jelinek wrote:

On Tue, Oct 23, 2012 at 02:35:24PM -0600, Jeff Law wrote:

+/* Return TRUE if the statement at the end of e-dest depends on
+   the output of any statement in BB.   Otherwise return FALSE.
+
+   This is used when we are threading a backedge and need to ensure
+   that temporary equivalences from BB do not affect the condition
+   in e-dest.  */
+
+static bool
+cond_arg_set_in_bb (edge e, basic_block bb, int n)
+{
+  ssa_op_iter iter;
+  use_operand_p use_p;
+  gimple last = gsi_stmt (gsi_last_bb (e-dest));


Use gimple last = last_stmt (e-dest); instead?  That way any possible
debug stmts are ignored.
I thought I'd already dealt with this before.  I'll double-check and 
take appropriate action.


Any opinions about pulling it into 4.7.x as that release is affected by 
this codegen bug?  I've got no strong opinions and I'm willing to pull 
it onto the branch if you want it.


jeff


Re: PR tree-optimization/54985

2012-10-23 Thread Jakub Jelinek
On Tue, Oct 23, 2012 at 03:21:59PM -0600, Jeff Law wrote:
 On 10/23/2012 02:50 PM, Jakub Jelinek wrote:
 +static bool
 +cond_arg_set_in_bb (edge e, basic_block bb, int n)
 +{
 +  ssa_op_iter iter;
 +  use_operand_p use_p;
 +  gimple last = gsi_stmt (gsi_last_bb (e-dest));
 
 Use gimple last = last_stmt (e-dest); instead?  That way any possible
 debug stmts are ignored.
 I thought I'd already dealt with this before.  I'll double-check and
 take appropriate action.
 
 Any opinions about pulling it into 4.7.x as that release is affected
 by this codegen bug?  I've got no strong opinions and I'm willing to
 pull it onto the branch if you want it.

I think it should be backported to 4.7, perhaps with a few days delay after the
trunk commit.

Jakub


Re: PR tree-optimization/54985

2012-10-23 Thread Jeff Law

On 10/23/2012 03:22 PM, Jakub Jelinek wrote:

On Tue, Oct 23, 2012 at 03:21:59PM -0600, Jeff Law wrote:

On 10/23/2012 02:50 PM, Jakub Jelinek wrote:

+static bool
+cond_arg_set_in_bb (edge e, basic_block bb, int n)
+{
+  ssa_op_iter iter;
+  use_operand_p use_p;
+  gimple last = gsi_stmt (gsi_last_bb (e-dest));


Use gimple last = last_stmt (e-dest); instead?  That way any possible
debug stmts are ignored.

I thought I'd already dealt with this before.  I'll double-check and
take appropriate action.

Any opinions about pulling it into 4.7.x as that release is affected
by this codegen bug?  I've got no strong opinions and I'm willing to
pull it onto the branch if you want it.


I think it should be backported to 4.7, perhaps with a few days delay after the
trunk commit.

Do we even have debug statements after control flow statements?

jeff



Re: PR tree-optimization/54985

2012-10-23 Thread Jakub Jelinek
On Tue, Oct 23, 2012 at 03:34:46PM -0600, Jeff Law wrote:
 I think it should be backported to 4.7, perhaps with a few days delay after 
 the
 trunk commit.
 Do we even have debug statements after control flow statements?

They shouldn't be there, so if you just give up the same way for gsi_stmt
NULL as well as non-control stmt, it shouldn't make a difference.
So last_stmt might be just shorter to type and more commonly used, nothing
more.

Jakub


Re: PR tree-optimization/54985

2012-10-23 Thread Sharad Singhai
The following trivial patch seems to fix it.

Index: tree-ssa-threadedge.c
===
--- tree-ssa-threadedge.c (revision 192749)
+++ tree-ssa-threadedge.c (working copy)
@@ -743,7 +743,7 @@
  safe to thread this edge.  */
   if (e-flags  EDGE_DFS_BACK)
 {
-  if (cond_arg_set_in_bb (e, e-dest, 1))
+  if (cond_arg_set_in_bb (e, e-dest))
  goto fail;
 }

@@ -787,7 +787,7 @@
  of threading without having to re-run DOM or VRP.  */
   if (dest
((e-flags  EDGE_DFS_BACK) == 0
-  || ! cond_arg_set_in_bb (taken_edge, e-dest, 2)))
+  || ! cond_arg_set_in_bb (taken_edge, e-dest)))
 {
   /* We don't want to thread back to a block we have already
  visited.  This may be overly conservative.  */
@@ -846,7 +846,7 @@
  do
   {
 if ((e-flags  EDGE_DFS_BACK) == 0
- || ! cond_arg_set_in_bb (e3, e-dest, 3))
+ || ! cond_arg_set_in_bb (e3, e-dest))
   e2 = thread_around_empty_block (e3,
   dummy_cond,
   handle_dominating_asserts,
Sharad


On Tue, Oct 23, 2012 at 4:48 PM, Sharad Singhai sing...@google.com wrote:
 The trunk seems to be broken at r192749 due to this patch.

 ../../srctrunk/gcc/tree-ssa-threadedge.c: In function ‘void
 thread_across_edge(gimple_statement_d*, edge_def*, bool,
 vec_ttree_node***, tree_node* (*)(gimple_statement_d*,
 gimple_statement_d*))’:
 ../../srctrunk/gcc/tree-ssa-threadedge.c:583: error: too many
 arguments to function ‘bool cond_arg_set_in_bb(edge_def*,
 basic_block_def*)’
 ../../srctrunk/gcc/tree-ssa-threadedge.c:746: error: at this point in file
 ../../srctrunk/gcc/tree-ssa-threadedge.c:583: error: too many
 arguments to function ‘bool cond_arg_set_in_bb(edge_def*,
 basic_block_def*)’
 ../../srctrunk/gcc/tree-ssa-threadedge.c:790: error: at this point in file
 ../../srctrunk/gcc/tree-ssa-threadedge.c:583: error: too many
 arguments to function ‘bool cond_arg_set_in_bb(edge_def*,
 basic_block_def*)’
 ../../srctrunk/gcc/tree-ssa-threadedge.c:849: error: at this point in file
 make: *** [tree-ssa-threadedge.o] Error 1

 Sharad