public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Improve DOM's optimization of control statements
@ 2015-09-30 22:11 Jeff Law
  2015-10-02 11:15 ` Renlin Li
  2015-10-07 22:03 ` Andreas Schwab
  0 siblings, 2 replies; 10+ messages in thread
From: Jeff Law @ 2015-09-30 22:11 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]


Until now DOM has had to be very conservative with handling control 
statements with known conditions.  This as been an unfortunate side 
effect of the interaction between removing edges and recycling names via 
the SSA_NAME manager.

Essentially DOM would have to leave control statements alone.  So you'd 
see stuff like

if (0 == 0)

left around by DOM.  The jump threader would thankfully come along and 
optimize that as a jump thread.  But that's terribly inefficient, not to 
mention it creates unnecessary churn in the CFG and SSA_NAMEs.

By optimizing that directly in DOM, including removing whatever edges 
are not executable, we no longer have to rely on jump threading to 
handle that case.  Less churn in the CFG & SSA_NAMEs.   There's also 
some chance for secondary optimizations with fewer edges left in the CFG 
for DOM to consider.

Unfortunately, the churn caused by jump threading made it excessively 
difficult to analyze before/after dumps.  Sadly, you can have the same 
code, but if the SSA_NAMEs have changed, that impacts coalescing as we 
leave SSA.  Churn in the CFG changes labels/jumps, often without 
changing the actual structure, etc.

I did some tests with valgrind to evaluate branching behaviour 
before/after effects on the resulting code and those effects were tiny, 
in the I doubt you could measure them range.  That was expected since 
what we're really doing here is just capturing the optimization earlier.

I had a couple more tests, but they were lost in a bit of idiocy.  The 
test included is the one I had a second copy of lying around.

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

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 6247 bytes --]

	* tree-ssa-dom.c (optimize_stmt): Collapse control flow statements
	with constant conditions.
	* tree-ssa-threadupdate.c (remove_jump_threads_starting_at): New.
	(remove_ctrl_stmt_and_useless_edges): No longer static.
	* tree-ssa-threadupdate.h (remove_jump_threads_starting_at): Prototype.
	(remove_ctrl_stmt_and_useless_edges): Likewise.

	* gcc.dg/tree-ssa/ssa-dom-branch-1.c: New test.

diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index 2c51e36..a8b7038 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -1820,31 +1820,8 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator si,
   if (is_gimple_assign (stmt))
     record_equivalences_from_stmt (stmt, may_optimize_p, avail_exprs_stack);
 
-  /* If STMT is a COND_EXPR and it was modified, then we may know
-     where it goes.  If that is the case, then mark the CFG as altered.
-
-     This will cause us to later call remove_unreachable_blocks and
-     cleanup_tree_cfg when it is safe to do so.  It is not safe to
-     clean things up here since removal of edges and such can trigger
-     the removal of PHI nodes, which in turn can release SSA_NAMEs to
-     the manager.
-
-     That's all fine and good, except that once SSA_NAMEs are released
-     to the manager, we must not call create_ssa_name until all references
-     to released SSA_NAMEs have been eliminated.
-
-     All references to the deleted SSA_NAMEs can not be eliminated until
-     we remove unreachable blocks.
-
-     We can not remove unreachable blocks until after we have completed
-     any queued jump threading.
-
-     We can not complete any queued jump threads until we have taken
-     appropriate variables out of SSA form.  Taking variables out of
-     SSA form can call create_ssa_name and thus we lose.
-
-     Ultimately I suspect we're going to need to change the interface
-     into the SSA_NAME manager.  */
+  /* If STMT is a COND_EXPR or SWITCH_EXPR and it was modified, then we may
+     know where it goes.  */
   if (gimple_modified_p (stmt) || modified_p)
     {
       tree val = NULL;
@@ -1858,8 +1835,27 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator si,
       else if (gswitch *swtch_stmt = dyn_cast <gswitch *> (stmt))
 	val = gimple_switch_index (swtch_stmt);
 
-      if (val && TREE_CODE (val) == INTEGER_CST && find_taken_edge (bb, val))
-	cfg_altered = true;
+      if (val && TREE_CODE (val) == INTEGER_CST)
+	{
+	  edge taken_edge = find_taken_edge (bb, val);
+	  if (taken_edge)
+	    {
+	      /* Delete threads that start at BB.  */
+	      remove_jump_threads_starting_at (bb);
+
+	      /* Now clean up the control statement at the end of
+		 BB and remove unexecutable edges.  */
+	      remove_ctrl_stmt_and_useless_edges (bb, taken_edge->dest);
+
+	      /* Fixup the flags on the single remaining edge.  */
+	      taken_edge->flags
+		&= ~(EDGE_TRUE_VALUE | EDGE_FALSE_VALUE | EDGE_ABNORMAL);
+	      taken_edge->flags |= EDGE_FALLTHRU;
+
+	      /* Further simplifications may be possible.  */
+	      cfg_altered = true;
+	    }
+	}
 
       /* If we simplified a statement in such a way as to be shown that it
 	 cannot trap, update the eh information and the cfg to match.  */
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index 6f21529..4a147bb 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -260,7 +260,7 @@ struct thread_stats_d thread_stats;
    Also remove all outgoing edges except the edge which reaches DEST_BB.
    If DEST_BB is NULL, then remove all outgoing edges.  */
 
-static void
+void
 remove_ctrl_stmt_and_useless_edges (basic_block bb, basic_block dest_bb)
 {
   gimple_stmt_iterator gsi;
@@ -2539,6 +2539,37 @@ valid_jump_thread_path (vec<jump_thread_edge *> *path)
   return true;
 }
 
+/* Remove any queued jump threads that start at BB.  */
+
+void
+remove_jump_threads_starting_at (basic_block bb)
+{
+  if (!paths.exists ())
+    return;
+
+  for (unsigned i = 0; i < paths.length ();)
+    {
+      vec<jump_thread_edge *> *path = paths[i];
+
+      /* Sadly, FSM jump threads have a slightly different
+	 representation than the rest of the jump threads.  */
+      if ((*path)[0]->type == EDGE_FSM_THREAD
+	  && (*path)[0]->e->src == bb)
+	{
+	  delete_jump_thread_path (path);
+	  paths.unordered_remove (i);
+	}
+      else if ((*path)[0]->type != EDGE_FSM_THREAD
+	       && (*path)[0]->e->dest == bb)
+	{
+	  delete_jump_thread_path (path);
+	  paths.unordered_remove (i);
+	}
+      else
+	i++;
+    }
+}
+
 /* Walk through all blocks and thread incoming edges to the appropriate
    outgoing edge for each edge pair recorded in THREADED_EDGES.
 
diff --git a/gcc/tree-ssa-threadupdate.h b/gcc/tree-ssa-threadupdate.h
index 21a9ee3..30428e8 100644
--- a/gcc/tree-ssa-threadupdate.h
+++ b/gcc/tree-ssa-threadupdate.h
@@ -43,5 +43,7 @@ public:
 };
 
 extern void register_jump_thread (vec <class jump_thread_edge *> *);
+extern void remove_jump_threads_starting_at (basic_block);
 extern void delete_jump_thread_path (vec <class jump_thread_edge *> *);
+extern void remove_ctrl_stmt_and_useless_edges (basic_block, basic_block);
 #endif
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-branch-1.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-branch-1.c
new file mode 100644
index 0000000..4c7631c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-branch-1.c
@@ -0,0 +1,31 @@
+/* { dg-do compile } */ 
+/* { dg-options "-O2 -w -fdump-tree-dom1-details" } */
+
+typedef struct rtx_def *rtx;
+struct rtx_def
+{
+  int code;
+  rtx rt_rtx;
+};
+rtx
+try_combine (rtx i1, rtx newpat)
+{
+  rtx temp;
+  if (i1 && (temp = ((((((newpat->rt_rtx, ((((temp)->code) == 42)))))))))
+      && ((temp =
+	(((((((((((newpat)->rt_rtx),
+		 ((((temp)->code) == 42) && arf ())))))))))))))
+    ;
+  else if (i1 && foo ());
+}
+
+/* There should be three tests against i1.  Two from the hash table
+   dumps, one in the code itself.  */
+/* { dg-final { scan-tree-dump-times "if .i1_" 3 "dom1"} } */
+
+/* There should be no actual jump threads realized by DOM.  The
+   legitimize jump threads are handled in VRP and those discovered
+   by DOM are subsumed by collapsing a conditional.  */
+/* { dg-final { scan-tree-dump-not "Threaded" "dom1"} } */
+
+

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-09-30 22:11 [PATCH] Improve DOM's optimization of control statements Jeff Law
@ 2015-10-02 11:15 ` Renlin Li
  2015-10-02 15:15   ` Jeff Law
                     ` (2 more replies)
  2015-10-07 22:03 ` Andreas Schwab
  1 sibling, 3 replies; 10+ messages in thread
From: Renlin Li @ 2015-10-02 11:15 UTC (permalink / raw)
  To: Jeff Law, gcc-patches; +Cc: Marcus Shawcroft

Hi Jeff,

Your patch causes an ICE regression.
The test case is " gcc.c-torture/compile/pr27087.c", I observed it on 
aarch64-none-elf target when compiling the test case with '-Os' flag.

A quick check shows, the cfg has been changed, but the loop information 
is not updated. Thus the information about the number of basic block in 
a loop is not reliable.

Could you please have a look?

Regards,
Renlin

On 30/09/15 21:28, Jeff Law wrote:
> Until now DOM has had to be very conservative with handling control
> statements with known conditions.  This as been an unfortunate side
> effect of the interaction between removing edges and recycling names via
> the SSA_NAME manager.
>
> Essentially DOM would have to leave control statements alone.  So you'd
> see stuff like
>
> if (0 == 0)
>
> left around by DOM.  The jump threader would thankfully come along and
> optimize that as a jump thread.  But that's terribly inefficient, not to
> mention it creates unnecessary churn in the CFG and SSA_NAMEs.
>
> By optimizing that directly in DOM, including removing whatever edges
> are not executable, we no longer have to rely on jump threading to
> handle that case.  Less churn in the CFG & SSA_NAMEs.   There's also
> some chance for secondary optimizations with fewer edges left in the CFG
> for DOM to consider.
>
> Unfortunately, the churn caused by jump threading made it excessively
> difficult to analyze before/after dumps.  Sadly, you can have the same
> code, but if the SSA_NAMEs have changed, that impacts coalescing as we
> leave SSA.  Churn in the CFG changes labels/jumps, often without
> changing the actual structure, etc.
>
> I did some tests with valgrind to evaluate branching behaviour
> before/after effects on the resulting code and those effects were tiny,
> in the I doubt you could measure them range.  That was expected since
> what we're really doing here is just capturing the optimization earlier.
>
> I had a couple more tests, but they were lost in a bit of idiocy.  The
> test included is the one I had a second copy of lying around.
>
> Bootstrapped and regression tested on x86_64-linux-gnu.  Installed on
> the trunk.
>
> Jeff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-02 11:15 ` Renlin Li
@ 2015-10-02 15:15   ` Jeff Law
  2015-10-02 16:25   ` Jeff Law
  2015-10-02 19:30   ` Jeff Law
  2 siblings, 0 replies; 10+ messages in thread
From: Jeff Law @ 2015-10-02 15:15 UTC (permalink / raw)
  To: Renlin Li, gcc-patches; +Cc: Marcus Shawcroft

On 10/02/2015 05:15 AM, Renlin Li wrote:
> Hi Jeff,
>
> Your patch causes an ICE regression.
> The test case is " gcc.c-torture/compile/pr27087.c", I observed it on
> aarch64-none-elf target when compiling the test case with '-Os' flag.
>
> A quick check shows, the cfg has been changed, but the loop information
> is not updated. Thus the information about the number of basic block in
> a loop is not reliable.
>
> Could you please have a look?
Yup.  Will do.   Thanks for letting me know.

jeff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-02 11:15 ` Renlin Li
  2015-10-02 15:15   ` Jeff Law
@ 2015-10-02 16:25   ` Jeff Law
  2015-10-02 19:30   ` Jeff Law
  2 siblings, 0 replies; 10+ messages in thread
From: Jeff Law @ 2015-10-02 16:25 UTC (permalink / raw)
  To: Renlin Li, gcc-patches; +Cc: Marcus Shawcroft

On 10/02/2015 05:15 AM, Renlin Li wrote:
> Hi Jeff,
>
> Your patch causes an ICE regression.
> The test case is " gcc.c-torture/compile/pr27087.c", I observed it on
> aarch64-none-elf target when compiling the test case with '-Os' flag.
>
> A quick check shows, the cfg has been changed, but the loop information
> is not updated. Thus the information about the number of basic block in
> a loop is not reliable.
>
> Could you please have a look?
Appears to be pretty simple.  If we collapse a conditional inside a 
loop, then we need to set the loop state as needing fixups.

In this specific case we have a conditional where one path eventually 
leads back to the loop latch block, the other path exits the loop.  We 
statically determine the conditional will always take us to the loop exit.

That has the side effect of making the block with the collapsed 
conditional no longer part of the loop, it's actually part of the exit 
path.  That changes the number of nodes in the loop, what edge(s) are 
the exit path(s) and possibly other stuff.

I'm running a fix through testing now.

Thanks,
jeff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-02 11:15 ` Renlin Li
  2015-10-02 15:15   ` Jeff Law
  2015-10-02 16:25   ` Jeff Law
@ 2015-10-02 19:30   ` Jeff Law
  2015-10-05  9:02     ` Richard Biener
  2 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2015-10-02 19:30 UTC (permalink / raw)
  To: Renlin Li, gcc-patches; +Cc: Marcus Shawcroft

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On 10/02/2015 05:15 AM, Renlin Li wrote:
> Hi Jeff,
>
> Your patch causes an ICE regression.
> The test case is " gcc.c-torture/compile/pr27087.c", I observed it on
> aarch64-none-elf target when compiling the test case with '-Os' flag.
>
> A quick check shows, the cfg has been changed, but the loop information
> is not updated. Thus the information about the number of basic block in
> a loop is not reliable.
>
> Could you please have a look?
As I mentioned, when we collapse a conditional inside a loop, we may 
change the # of nodes in a loop which edges are exit edges and possibly 
other stuff.  So we need to mark loops as needing fixups.

Verified this fixes the aarch64-elf regression and did a bootstrap & 
regression test on x86_64-linux-gnu.

Installed on the trunk.

jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1377 bytes --]

commit 992d281b2d1ba53a49198db44fee92a505e16f5d
Author: Jeff Law <law@tor.usersys.redhat.com>
Date:   Fri Oct 2 15:22:04 2015 -0400

    Re: [PATCH] Improve DOM's optimization of control statements
    
    	* tree-ssa-dom.c (optimize_stmt): Note when loop structures need
    	fixups.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3f7561a..e541df3 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2015-10-02  Jeff Law  <law@redhat.com>
+
+	* tree-ssa-dom.c (optimize_stmt): Note when loop structures need
+	fixups.
+
 2015-10-02  Uros Bizjak  <ubizjak@gmail.com>
 
 	* system.h (ROUND_UP): New macro definition.
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index a8b7038..d940816 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -1843,6 +1843,12 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator si,
 	      /* Delete threads that start at BB.  */
 	      remove_jump_threads_starting_at (bb);
 
+	      /* If BB is in a loop, then removing an outgoing edge from BB
+		 may cause BB to move outside the loop, changes in the
+		 loop exit edges, etc.  So note that loops need fixing.  */
+	      if (bb_loop_depth (bb) > 0)
+		loops_state_set (LOOPS_NEED_FIXUP);
+
 	      /* Now clean up the control statement at the end of
 		 BB and remove unexecutable edges.  */
 	      remove_ctrl_stmt_and_useless_edges (bb, taken_edge->dest);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-02 19:30   ` Jeff Law
@ 2015-10-05  9:02     ` Richard Biener
  2015-10-06 17:41       ` Jeff Law
  2015-10-07 21:56       ` Jeff Law
  0 siblings, 2 replies; 10+ messages in thread
From: Richard Biener @ 2015-10-05  9:02 UTC (permalink / raw)
  To: Jeff Law; +Cc: Renlin Li, gcc-patches, Marcus Shawcroft

On Fri, Oct 2, 2015 at 9:30 PM, Jeff Law <law@redhat.com> wrote:
> On 10/02/2015 05:15 AM, Renlin Li wrote:
>>
>> Hi Jeff,
>>
>> Your patch causes an ICE regression.
>> The test case is " gcc.c-torture/compile/pr27087.c", I observed it on
>> aarch64-none-elf target when compiling the test case with '-Os' flag.
>>
>> A quick check shows, the cfg has been changed, but the loop information
>> is not updated. Thus the information about the number of basic block in
>> a loop is not reliable.
>>
>> Could you please have a look?
>
> As I mentioned, when we collapse a conditional inside a loop, we may change
> the # of nodes in a loop which edges are exit edges and possibly other
> stuff.  So we need to mark loops as needing fixups.
>
> Verified this fixes the aarch64-elf regression and did a bootstrap &
> regression test on x86_64-linux-gnu.
>
> Installed on the trunk.
>
> jeff
>
> commit 992d281b2d1ba53a49198db44fee92a505e16f5d
> Author: Jeff Law <law@tor.usersys.redhat.com>
> Date:   Fri Oct 2 15:22:04 2015 -0400
>
>     Re: [PATCH] Improve DOM's optimization of control statements
>
>         * tree-ssa-dom.c (optimize_stmt): Note when loop structures need
>         fixups.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 3f7561a..e541df3 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,8 @@
> +2015-10-02  Jeff Law  <law@redhat.com>
> +
> +       * tree-ssa-dom.c (optimize_stmt): Note when loop structures need
> +       fixups.
> +
>  2015-10-02  Uros Bizjak  <ubizjak@gmail.com>
>
>         * system.h (ROUND_UP): New macro definition.
> diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
> index a8b7038..d940816 100644
> --- a/gcc/tree-ssa-dom.c
> +++ b/gcc/tree-ssa-dom.c
> @@ -1843,6 +1843,12 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator
> si,
>               /* Delete threads that start at BB.  */
>               remove_jump_threads_starting_at (bb);
>
> +             /* If BB is in a loop, then removing an outgoing edge from BB
> +                may cause BB to move outside the loop, changes in the
> +                loop exit edges, etc.  So note that loops need fixing.  */
> +             if (bb_loop_depth (bb) > 0)
> +               loops_state_set (LOOPS_NEED_FIXUP);
> +

I would rather do this in remove_ctrl_stmt_and_useless_edges and only
if taken_edge is a loop exit.  loop fixup is a pretty big hammer which
we should avoid at all cost.

So please try to be more specific on the cases you invoke it.

Thanks,
Richard.

>               /* Now clean up the control statement at the end of
>                  BB and remove unexecutable edges.  */
>               remove_ctrl_stmt_and_useless_edges (bb, taken_edge->dest);
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-05  9:02     ` Richard Biener
@ 2015-10-06 17:41       ` Jeff Law
  2015-10-07 21:56       ` Jeff Law
  1 sibling, 0 replies; 10+ messages in thread
From: Jeff Law @ 2015-10-06 17:41 UTC (permalink / raw)
  To: Richard Biener; +Cc: Renlin Li, gcc-patches, Marcus Shawcroft

On 10/05/2015 03:02 AM, Richard Biener wrote:
>> +             /* If BB is in a loop, then removing an outgoing edge from BB
>> +                may cause BB to move outside the loop, changes in the
>> +                loop exit edges, etc.  So note that loops need fixing.  */
>> +             if (bb_loop_depth (bb) > 0)
>> +               loops_state_set (LOOPS_NEED_FIXUP);
>> +
>
> I would rather do this in remove_ctrl_stmt_and_useless_edges and only
> if taken_edge is a loop exit.  loop fixup is a pretty big hammer which
> we should avoid at all cost.
remove_ctrl_stmt_and_useless_edges is used internally when duplicating 
blocks for jump threading, so we'd need to audit for that case as well.

>
> So please try to be more specific on the cases you invoke it.
I'd pondered this and was still working through whether or not 
triggering only when one of the outgoing edges was "interesting" from a 
loop structure standpoint.  I hadn't tested that yet and wanted to get 
the regression resolved, hence the large hammer.


jeff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-05  9:02     ` Richard Biener
  2015-10-06 17:41       ` Jeff Law
@ 2015-10-07 21:56       ` Jeff Law
  1 sibling, 0 replies; 10+ messages in thread
From: Jeff Law @ 2015-10-07 21:56 UTC (permalink / raw)
  To: Richard Biener; +Cc: Renlin Li, gcc-patches, Marcus Shawcroft

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

On 10/05/2015 03:02 AM, Richard Biener wrote:
> On Fri, Oct 2, 2015 at 9:30 PM, Jeff Law <law@redhat.com> wrote:
>> On 10/02/2015 05:15 AM, Renlin Li wrote:
>>>
>>> Hi Jeff,
>>>
>>> Your patch causes an ICE regression.
>>> The test case is " gcc.c-torture/compile/pr27087.c", I observed it on
>>> aarch64-none-elf target when compiling the test case with '-Os' flag.
>>>
>>> A quick check shows, the cfg has been changed, but the loop information
>>> is not updated. Thus the information about the number of basic block in
>>> a loop is not reliable.
>>>
>>> Could you please have a look?
>>
>> As I mentioned, when we collapse a conditional inside a loop, we may change
>> the # of nodes in a loop which edges are exit edges and possibly other
>> stuff.  So we need to mark loops as needing fixups.
>>
>> Verified this fixes the aarch64-elf regression and did a bootstrap &
>> regression test on x86_64-linux-gnu.
>>
>> Installed on the trunk.
>>
>> jeff
>>
>> commit 992d281b2d1ba53a49198db44fee92a505e16f5d
>> Author: Jeff Law <law@tor.usersys.redhat.com>
>> Date:   Fri Oct 2 15:22:04 2015 -0400
>>
>>      Re: [PATCH] Improve DOM's optimization of control statements
>>
>>          * tree-ssa-dom.c (optimize_stmt): Note when loop structures need
>>          fixups.
>>
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index 3f7561a..e541df3 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2015-10-02  Jeff Law  <law@redhat.com>
>> +
>> +       * tree-ssa-dom.c (optimize_stmt): Note when loop structures need
>> +       fixups.
>> +
>>   2015-10-02  Uros Bizjak  <ubizjak@gmail.com>
>>
>>          * system.h (ROUND_UP): New macro definition.
>> diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
>> index a8b7038..d940816 100644
>> --- a/gcc/tree-ssa-dom.c
>> +++ b/gcc/tree-ssa-dom.c
>> @@ -1843,6 +1843,12 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator
>> si,
>>                /* Delete threads that start at BB.  */
>>                remove_jump_threads_starting_at (bb);
>>
>> +             /* If BB is in a loop, then removing an outgoing edge from BB
>> +                may cause BB to move outside the loop, changes in the
>> +                loop exit edges, etc.  So note that loops need fixing.  */
>> +             if (bb_loop_depth (bb) > 0)
>> +               loops_state_set (LOOPS_NEED_FIXUP);
>> +
>
> I would rather do this in remove_ctrl_stmt_and_useless_edges and only
> if taken_edge is a loop exit.  loop fixup is a pretty big hammer which
> we should avoid at all cost.
>
> So please try to be more specific on the cases you invoke it.
What's probably the most interesting is we don't actually have 
EDGE_LOOP_EXIT set in DOM.   So we can't use that, but that also implies 
it doesn't need updating.  Thankfully, there's an alternate test we can 
use instead.

And after more ponderings, I'm pretty sure the only case that's of 
concern right now is nodes moving out of the loop, which only happens 
when we have a BB with a loop exit edge and we delete the *other* edges. 
  We've got simple latches & preheaders and I don't think we destroy 
that property.   Both properties also simplify the things we need look for.

So I've minimized use of the hammer to just the case where blocks are 
going to be moving out of the loop.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu and 
verified the ARM test which originally spurred this change continues to 
work.

Installed on the trunk.

Jeff

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1779 bytes --]

	* tree-ssa-dom.c (optimize_stmt): Don't set LOOPS_NEED_FIXUP here.
	* tree-ssa-threadupdate.c (remove_ctrl_stmt_and_useless_edges): Do it
	here instead.  Tighten test to avoid setting LOOPS_NEED_FIXUP 
	unnecessarily.

diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index 941087d..38cceff 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -1848,12 +1848,6 @@ optimize_stmt (basic_block bb, gimple_stmt_iterator si,
 	      FOR_EACH_EDGE (e, ei, bb->succs)
 		remove_jump_threads_including (e);
 
-	      /* If BB is in a loop, then removing an outgoing edge from BB
-		 may cause BB to move outside the loop, changes in the
-		 loop exit edges, etc.  So note that loops need fixing.  */
-	      if (bb_loop_depth (bb) > 0)
-		loops_state_set (LOOPS_NEED_FIXUP);
-
 	      /* Now clean up the control statement at the end of
 		 BB and remove unexecutable edges.  */
 	      remove_ctrl_stmt_and_useless_edges (bb, taken_edge->dest);
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index 26b199b..e426c1d 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -300,6 +300,17 @@ remove_ctrl_stmt_and_useless_edges (basic_block bb, basic_block dest_bb)
       else
 	ei_next (&ei);
     }
+
+  /* If the remaining edge is a loop exit, there must have
+     a removed edge that was not a loop exit.
+
+     In that case BB and possibly other blocks were previously
+     in the loop, but are now outside the loop.  Thus, we need
+     to update the loop structures.  */
+  if (single_succ_p (bb)
+      && loop_outer (bb->loop_father)
+      && loop_exit_edge_p (bb->loop_father, single_succ_edge (bb)))
+    loops_state_set (LOOPS_NEED_FIXUP);
 }
 
 /* Create a duplicate of BB.  Record the duplicate block in an array

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-09-30 22:11 [PATCH] Improve DOM's optimization of control statements Jeff Law
  2015-10-02 11:15 ` Renlin Li
@ 2015-10-07 22:03 ` Andreas Schwab
  2015-10-07 22:06   ` Jeff Law
  1 sibling, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2015-10-07 22:03 UTC (permalink / raw)
  To: Jeff Law; +Cc: gcc-patches

On powerpc:

FAIL: gcc.c-torture/compile/pr52073.c   -O2  (internal compiler error)

/daten/gcc/gcc-20151006/gcc/testsuite/gcc.c-torture/compile/pr52073.c:6:1: internal compiler error: in duplicate_thread_path, at tree-ssa-threadupdate.c:2446.
0x10874ab7 duplicate_thread_path.
	../../gcc/tree-ssa-threadupdate.c:2445.
0x10874ab7 thread_through_all_blocks(bool).
	../../gcc/tree-ssa-threadupdate.c:2632.
0x107b0ea7 execute.
	../../gcc/tree-ssa-dom.c:622.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Improve DOM's optimization of control statements
  2015-10-07 22:03 ` Andreas Schwab
@ 2015-10-07 22:06   ` Jeff Law
  0 siblings, 0 replies; 10+ messages in thread
From: Jeff Law @ 2015-10-07 22:06 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: gcc-patches

On 10/07/2015 04:03 PM, Andreas Schwab wrote:
> On powerpc:
>
> FAIL: gcc.c-torture/compile/pr52073.c   -O2  (internal compiler error)
>
> /daten/gcc/gcc-20151006/gcc/testsuite/gcc.c-torture/compile/pr52073.c:6:1: internal compiler error: in duplicate_thread_path, at tree-ssa-threadupdate.c:2446.
> 0x10874ab7 duplicate_thread_path.
> 	../../gcc/tree-ssa-threadupdate.c:2445.
> 0x10874ab7 thread_through_all_blocks(bool).
> 	../../gcc/tree-ssa-threadupdate.c:2632.
> 0x107b0ea7 execute.
> 	../../gcc/tree-ssa-dom.c:622.
Update your tree.  This should have been fixed by:
2015-10-06  Jeff Law  <law@redhat.com>

         PR tree-optimization/67816

Jeff

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-10-07 22:06 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-30 22:11 [PATCH] Improve DOM's optimization of control statements Jeff Law
2015-10-02 11:15 ` Renlin Li
2015-10-02 15:15   ` Jeff Law
2015-10-02 16:25   ` Jeff Law
2015-10-02 19:30   ` Jeff Law
2015-10-05  9:02     ` Richard Biener
2015-10-06 17:41       ` Jeff Law
2015-10-07 21:56       ` Jeff Law
2015-10-07 22:03 ` Andreas Schwab
2015-10-07 22:06   ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).