diff mbox series

[SFN+LVU+IEPM,v4,4/9,SFN] stabilize find_bb_boundaries

Message ID 20171110023448.28164-4-aoliva@redhat.com
State New
Headers show
Series [SFN+LVU+IEPM,v4,1/9,SFN] adjust RTL insn-walking API | expand

Commit Message

Alexandre Oliva Nov. 10, 2017, 2:34 a.m. UTC
If find_bb_boundaries is given a block with zero or one nondebug insn
beside debug insns, it shouldn't purge dead edges, because without
debug insns we wouldn't purge them at that point.  Doing so may change
the order in which edges are processed, and ultimately lead to
different transformations to the CFG and then to different
optimizations.

We shouldn't, however, retain debug insns after control flow insns, so
if we find debug insns after a single insn that happens to be a
control flow insn, do the debug insn cleanups, but still refrain from
purging dead edges at that point.


for  gcc/ChangeLog

	* cfgbuild.c (find_bb_boundaries): Don't purge dead edges if,
	without debug insns, we wouldn't, but clean up debug insns
	after a control flow insn nevertheless.
---
 gcc/cfgbuild.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Comments

Jeff Law Dec. 7, 2017, 10:46 p.m. UTC | #1
On 11/09/2017 07:34 PM, Alexandre Oliva wrote:
> If find_bb_boundaries is given a block with zero or one nondebug insn
> beside debug insns, it shouldn't purge dead edges, because without
> debug insns we wouldn't purge them at that point.  Doing so may change
> the order in which edges are processed, and ultimately lead to
> different transformations to the CFG and then to different
> optimizations.
> 
> We shouldn't, however, retain debug insns after control flow insns, so
> if we find debug insns after a single insn that happens to be a
> control flow insn, do the debug insn cleanups, but still refrain from
> purging dead edges at that point.
> 
> 
> for  gcc/ChangeLog
> 
> 	* cfgbuild.c (find_bb_boundaries): Don't purge dead edges if,
> 	without debug insns, we wouldn't, but clean up debug insns
> 	after a control flow insn nevertheless.
OK.  Seems to me like it's independent of the rest of the work and
should go in immediately.

jeff
Alexandre Oliva Dec. 12, 2017, 2:37 a.m. UTC | #2
On Dec  7, 2017, Jeff Law <law@redhat.com> wrote:

> On 11/09/2017 07:34 PM, Alexandre Oliva wrote:
>> * cfgbuild.c (find_bb_boundaries): Don't purge dead edges if,
>> without debug insns, we wouldn't, but clean up debug insns
>> after a control flow insn nevertheless.
> OK.  Seems to me like it's independent of the rest of the work and
> should go in immediately.

Yeah, I wasn't even sure whether to submit it as part of the SFN
patchset.  IIRC it was a build regression of the patchset, that only
came up after I'd first posted it, and I could only trigger it with the
patchset, IIRC because it involved debug insns at places where only
these patches would insert them.  So I kept it in the set, but as a
separate patch.

Anyway, thanks, here it is as installed, FTR:

From 116cfb8c5abdcd64333be8fa105a4af2dfd13823 Mon Sep 17 00:00:00 2001
From: aoliva <aoliva@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Tue, 12 Dec 2017 02:15:44 +0000
Subject: [PATCH 4/7] [SFN] stabilize find_bb_boundaries

If find_bb_boundaries is given a block with zero or one nondebug insn
beside debug insns, it shouldn't purge dead edges, because without
debug insns we wouldn't purge them at that point.  Doing so may change
the order in which edges are processed, and ultimately lead to
different transformations to the CFG and then to different
optimizations.

We shouldn't, however, retain debug insns after control flow insns, so
if we find debug insns after a single insn that happens to be a
control flow insn, do the debug insn cleanups, but still refrain from
purging dead edges at that point.


for  gcc/ChangeLog

	* cfgbuild.c (find_bb_boundaries): Don't purge dead edges if,
	without debug insns, we wouldn't, but clean up debug insns
	after a control flow insn nevertheless.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@255567 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog  |  4 ++++
 gcc/cfgbuild.c | 40 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 03ad41c3e27a..3c1add62c944 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,9 @@
 2017-12-12  Alexandre Oliva <aoliva@redhat.com>
 
+	* cfgbuild.c (find_bb_boundaries): Don't purge dead edges if,
+	without debug insns, we wouldn't, but clean up debug insns
+	after a control flow insn nevertheless.
+
 	* cfgcleanup.c (delete_unreachable_blocks): Use alternate
 	block removal order if MAY_HAVE_DEBUG_BIND_INSNS.
 	* cfgexpand.c (label_rtx_for_bb): Skip debug insns.
diff --git a/gcc/cfgbuild.c b/gcc/cfgbuild.c
index 8fa15fec45e4..7b57589f1dfa 100644
--- a/gcc/cfgbuild.c
+++ b/gcc/cfgbuild.c
@@ -444,10 +444,43 @@ find_bb_boundaries (basic_block bb)
   rtx_insn *flow_transfer_insn = NULL;
   rtx_insn *debug_insn = NULL;
   edge fallthru = NULL;
+  bool skip_purge;
 
   if (insn == end)
     return;
 
+  if (DEBUG_INSN_P (insn) || DEBUG_INSN_P (end))
+    {
+      /* Check whether, without debug insns, the insn==end test above
+	 would have caused us to return immediately, and behave the
+	 same way even with debug insns.  If we don't do this, debug
+	 insns could cause us to purge dead edges at different times,
+	 which could in turn change the cfg and affect codegen
+	 decisions in subtle but undesirable ways.  */
+      while (insn != end && DEBUG_INSN_P (insn))
+	insn = NEXT_INSN (insn);
+      rtx_insn *e = end;
+      while (insn != e && DEBUG_INSN_P (e))
+	e = PREV_INSN (e);
+      if (insn == e)
+	{
+	  /* If there are debug insns after a single insn that is a
+	     control flow insn in the block, we'd have left right
+	     away, but we should clean up the debug insns after the
+	     control flow insn, because they can't remain in the same
+	     block.  So, do the debug insn cleaning up, but then bail
+	     out without purging dead edges as we would if the debug
+	     insns hadn't been there.  */
+	  if (e != end && !DEBUG_INSN_P (e) && control_flow_insn_p (e))
+	    {
+	      skip_purge = true;
+	      flow_transfer_insn = e;
+	      goto clean_up_debug_after_control_flow;
+	    }
+	  return;
+	}
+    }
+
   if (LABEL_P (insn))
     insn = NEXT_INSN (insn);
 
@@ -475,7 +508,6 @@ find_bb_boundaries (basic_block bb)
 	  if (debug_insn && code != CODE_LABEL && code != BARRIER)
 	    prev = PREV_INSN (debug_insn);
 	  fallthru = split_block (bb, prev);
-
 	  if (flow_transfer_insn)
 	    {
 	      BB_END (bb) = flow_transfer_insn;
@@ -527,6 +559,9 @@ find_bb_boundaries (basic_block bb)
      ordinary jump, we need to take care and move basic block boundary.  */
   if (flow_transfer_insn && flow_transfer_insn != end)
     {
+      skip_purge = false;
+
+    clean_up_debug_after_control_flow:
       BB_END (bb) = flow_transfer_insn;
 
       /* Clean up the bb field for the insns that do not belong to BB.  */
@@ -543,6 +578,9 @@ find_bb_boundaries (basic_block bb)
 	  if (x == end)
 	    break;
 	}
+
+      if (skip_purge)
+	return;
     }
 
   /* We've possibly replaced the conditional jump by conditional jump
diff mbox series

Patch

diff --git a/gcc/cfgbuild.c b/gcc/cfgbuild.c
index 8fa15fec45e4..7b57589f1dfa 100644
--- a/gcc/cfgbuild.c
+++ b/gcc/cfgbuild.c
@@ -444,10 +444,43 @@  find_bb_boundaries (basic_block bb)
   rtx_insn *flow_transfer_insn = NULL;
   rtx_insn *debug_insn = NULL;
   edge fallthru = NULL;
+  bool skip_purge;
 
   if (insn == end)
     return;
 
+  if (DEBUG_INSN_P (insn) || DEBUG_INSN_P (end))
+    {
+      /* Check whether, without debug insns, the insn==end test above
+	 would have caused us to return immediately, and behave the
+	 same way even with debug insns.  If we don't do this, debug
+	 insns could cause us to purge dead edges at different times,
+	 which could in turn change the cfg and affect codegen
+	 decisions in subtle but undesirable ways.  */
+      while (insn != end && DEBUG_INSN_P (insn))
+	insn = NEXT_INSN (insn);
+      rtx_insn *e = end;
+      while (insn != e && DEBUG_INSN_P (e))
+	e = PREV_INSN (e);
+      if (insn == e)
+	{
+	  /* If there are debug insns after a single insn that is a
+	     control flow insn in the block, we'd have left right
+	     away, but we should clean up the debug insns after the
+	     control flow insn, because they can't remain in the same
+	     block.  So, do the debug insn cleaning up, but then bail
+	     out without purging dead edges as we would if the debug
+	     insns hadn't been there.  */
+	  if (e != end && !DEBUG_INSN_P (e) && control_flow_insn_p (e))
+	    {
+	      skip_purge = true;
+	      flow_transfer_insn = e;
+	      goto clean_up_debug_after_control_flow;
+	    }
+	  return;
+	}
+    }
+
   if (LABEL_P (insn))
     insn = NEXT_INSN (insn);
 
@@ -475,7 +508,6 @@  find_bb_boundaries (basic_block bb)
 	  if (debug_insn && code != CODE_LABEL && code != BARRIER)
 	    prev = PREV_INSN (debug_insn);
 	  fallthru = split_block (bb, prev);
-
 	  if (flow_transfer_insn)
 	    {
 	      BB_END (bb) = flow_transfer_insn;
@@ -527,6 +559,9 @@  find_bb_boundaries (basic_block bb)
      ordinary jump, we need to take care and move basic block boundary.  */
   if (flow_transfer_insn && flow_transfer_insn != end)
     {
+      skip_purge = false;
+
+    clean_up_debug_after_control_flow:
       BB_END (bb) = flow_transfer_insn;
 
       /* Clean up the bb field for the insns that do not belong to BB.  */
@@ -543,6 +578,9 @@  find_bb_boundaries (basic_block bb)
 	  if (x == end)
 	    break;
 	}
+
+      if (skip_purge)
+	return;
     }
 
   /* We've possibly replaced the conditional jump by conditional jump