[SFN] Bootstrap broken

[SFN] Bootstrap broken

[David Edelsohn]

Something in this series broke bootstrap on AIX, probably Power in general.

- David

/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c: In function 'void
pp_gimple_stmt_1(pretty_printer*, gimple*, int, dump_flags_t)':
/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: error:
invalid rtl sharing found in the insn
 }
 ^
(debug_insn 5082 5081 5083 (var_location:DI flags (concatn/v:DI [
            (reg:SI 1564 [ flags ])
            (reg:SI 1565 [ flags+4 ])
        ])) "/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c":2583 -1
     (nil))
/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: error: shared rtx
(concatn/v:DI [
        (reg:SI 1564 [ flags ])
        (reg:SI 1565 [ flags+4 ])
    ])
during RTL pass: outof_cfglayout
/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: internal
compiler error: internal consistency failure

Re: [SFN] Bootstrap broken

[Rainer Orth]

Hi David,

> Something in this series broke bootstrap on AIX, probably Power in general.

I'm seeing the same in a sparc-sun-solaris2.11 bootstrap.

	Rainer


> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c: In function 'void
> pp_gimple_stmt_1(pretty_printer*, gimple*, int, dump_flags_t)':
> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: error:
> invalid rtl sharing found in the insn
>  }
>  ^
> (debug_insn 5082 5081 5083 (var_location:DI flags (concatn/v:DI [
>             (reg:SI 1564 [ flags ])
>             (reg:SI 1565 [ flags+4 ])
>         ])) "/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c":2583 -1
>      (nil))
> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: error: shared rtx
> (concatn/v:DI [
>         (reg:SI 1564 [ flags ])
>         (reg:SI 1565 [ flags+4 ])
>     ])
> during RTL pass: outof_cfglayout
> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: internal
> compiler error: internal consistency failure

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [SFN] Bootstrap broken

[David Edelsohn]

Rainer,

PR83396 opened.  you can add Solaris to the list of targets.

- David

On Tue, Dec 12, 2017 at 1:49 PM, Rainer Orth
<ro@cebitec.uni-bielefeld.de> wrote:
> Hi David,
>
>> Something in this series broke bootstrap on AIX, probably Power in general.
>
> I'm seeing the same in a sparc-sun-solaris2.11 bootstrap.
>
>         Rainer
>
>
>> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c: In function 'void
>> pp_gimple_stmt_1(pretty_printer*, gimple*, int, dump_flags_t)':
>> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: error:
>> invalid rtl sharing found in the insn
>>  }
>>  ^
>> (debug_insn 5082 5081 5083 (var_location:DI flags (concatn/v:DI [
>>             (reg:SI 1564 [ flags ])
>>             (reg:SI 1565 [ flags+4 ])
>>         ])) "/nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c":2583 -1
>>      (nil))
>> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: error: shared rtx
>> (concatn/v:DI [
>>         (reg:SI 1564 [ flags ])
>>         (reg:SI 1565 [ flags+4 ])
>>     ])
>> during RTL pass: outof_cfglayout
>> /nasfarm/edelsohn/src/src/gcc/gimple-pretty-print.c:2645:1: internal
>> compiler error: internal consistency failure
>
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 12, 2017, David Edelsohn <dje.gcc@gmail.com> wrote:

> Rainer,
> PR83396 opened.  you can add Solaris to the list of targets.

Andreas,

Here's a fix for the ia64 regression you mentioned in that PR.

I don't include the 'int main(){return 0;}' testcase, because it would
hardly be useful; any test would trigger this error on the right
platform.

Regstrapping; I suppose I could install it as obvious, but...  Ok to install?


[SFN] don't eliminate regs in markers

Eliminate regs in debug bind insns, but not in markers.

for  gcc/ChangeLog

	PR bootstrap/83396
	* reload1.c (eliminate_regs_in_insn): Skip debug markers.
---
 gcc/reload1.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/reload1.c b/gcc/reload1.c
index 322696a25f3e..fe1ec0d011fb 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -3202,7 +3202,7 @@ eliminate_regs_in_insn (rtx_insn *insn, int replace)
 		  || GET_CODE (PATTERN (insn)) == USE
 		  || GET_CODE (PATTERN (insn)) == CLOBBER
 		  || GET_CODE (PATTERN (insn)) == ASM_INPUT);
-      if (DEBUG_INSN_P (insn))
+      if (DEBUG_BIND_INSN_P (insn))
 	INSN_VAR_LOCATION_LOC (insn)
 	  = eliminate_regs (INSN_VAR_LOCATION_LOC (insn), VOIDmode, insn);
       return 0;


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 05:29:28AM -0200, Alexandre Oliva wrote:
> Regstrapping; I suppose I could install it as obvious, but...  Ok to install?
> 
> 
> [SFN] don't eliminate regs in markers
> 
> Eliminate regs in debug bind insns, but not in markers.
> 
> for  gcc/ChangeLog
> 
> 	PR bootstrap/83396
> 	* reload1.c (eliminate_regs_in_insn): Skip debug markers.

Ok.

> --- a/gcc/reload1.c
> +++ b/gcc/reload1.c
> @@ -3202,7 +3202,7 @@ eliminate_regs_in_insn (rtx_insn *insn, int replace)
>  		  || GET_CODE (PATTERN (insn)) == USE
>  		  || GET_CODE (PATTERN (insn)) == CLOBBER
>  		  || GET_CODE (PATTERN (insn)) == ASM_INPUT);
> -      if (DEBUG_INSN_P (insn))
> +      if (DEBUG_BIND_INSN_P (insn))
>  	INSN_VAR_LOCATION_LOC (insn)
>  	  = eliminate_regs (INSN_VAR_LOCATION_LOC (insn), VOIDmode, insn);
>        return 0;

	Jakub

Re: [SFN] Bootstrap broken

[Andreas Schwab]

This fixes the m68k ICE.

Andreas.

	PR bootstrap/83396
	* reload1.c (emit_input_reload_insns): Skip debug markers.

---
 gcc/reload1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/reload1.c b/gcc/reload1.c
index fe1ec0d011..baedc43b75 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -7345,12 +7345,12 @@ emit_input_reload_insns (struct insn_chain *chain, struct reload *rl,
 
 	      /* Adjust any debug insns between temp and insn.  */
 	      while ((temp = NEXT_INSN (temp)) != insn)
-		if (DEBUG_INSN_P (temp))
+		if (DEBUG_BIND_INSN_P (temp))
 		  INSN_VAR_LOCATION_LOC (temp)
 		    = simplify_replace_rtx (INSN_VAR_LOCATION_LOC (temp),
 					    old, reloadreg);
 		else
-		  gcc_assert (NOTE_P (temp));
+		  gcc_assert (DEBUG_INSN_P (temp) || NOTE_P (temp));
 	    }
 	  else
 	    {
-- 
2.15.1

-- 
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."

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Thu, Dec 14, 2017 at 03:41:24PM +0100, Andreas Schwab wrote:
> This fixes the m68k ICE.
> 
> Andreas.
> 
> 	PR bootstrap/83396
> 	* reload1.c (emit_input_reload_insns): Skip debug markers.

This is ok for trunk, thanks.

> --- a/gcc/reload1.c
> +++ b/gcc/reload1.c
> @@ -7345,12 +7345,12 @@ emit_input_reload_insns (struct insn_chain *chain, struct reload *rl,
>  
>  	      /* Adjust any debug insns between temp and insn.  */
>  	      while ((temp = NEXT_INSN (temp)) != insn)
> -		if (DEBUG_INSN_P (temp))
> +		if (DEBUG_BIND_INSN_P (temp))
>  		  INSN_VAR_LOCATION_LOC (temp)
>  		    = simplify_replace_rtx (INSN_VAR_LOCATION_LOC (temp),
>  					    old, reloadreg);
>  		else
> -		  gcc_assert (NOTE_P (temp));
> +		  gcc_assert (DEBUG_INSN_P (temp) || NOTE_P (temp));
>  	    }
>  	  else
>  	    {

	Jakub

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 13, 2017, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Dec 12, 2017, David Edelsohn <dje.gcc@gmail.com> wrote:
>> Rainer,
>> PR83396 opened.  you can add Solaris to the list of targets.

> Andreas,

> Here's a fix for the ia64 regression you mentioned in that PR.

And here's a patch that fixes the two other ia64 build failures you'd
mentioned.

Regstrapped on x86_64- and i686-linux-gnu by myself; bootstrapped on
ia64-linux-gnu by yourself IIUC, though with some weird bootstrap
compare errors I'm very curious to learn more about.

Ok to install?

Emitting markers before labels turned out to not be worth the trouble.
The markers outside BBs confuse the ebb scheduler, and they don't add
any useful information.  I'll arrange for markers to be moved past
labels, even in gimple, but for now this will fix the two remaining
known problems on ia64.

for  gcc/ChangeLog

	PR bootstrap/83396
	* cfgexpand.c (expand_gimple_basic_block): Expand label first,
	even if there are markers before it.
	* cfgrtl.c (rtl_verify_bb_layout): Reject DEBUG_INSNs outside BBs.
---
 gcc/cfgexpand.c |   12 ++++--------
 gcc/cfgrtl.c    |    1 -
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index ce98264214ae..30d9bac1118e 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -5510,20 +5510,16 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
   gimple *label_stmt = stmt;
   rtx_code_label **elt = lab_rtx_for_bb->get (bb);
 
-  if (stmt)
-    /* We'll get to it in the loop below, and get back to
-       emit_label_and_note then.  */
-    ;
-  else if (stmt || elt)
+  if (stmt || elt)
     {
-    emit_label_and_note:
       gcc_checking_assert (!note);
       last = get_last_insn ();
 
       if (stmt)
 	{
 	  expand_gimple_stmt (stmt);
-	  gsi_next (&gsi);
+	  if (gsi_stmt (gsi) == stmt)
+	    gsi_next (&gsi);
 	}
 
       if (elt)
@@ -5550,7 +5546,7 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
       stmt = gsi_stmt (gsi);
 
       if (stmt == label_stmt)
-	goto emit_label_and_note;
+	continue;
 
       /* If this statement is a non-debug one, and we generate debug
 	 insns, then this one might be the last real use of a TERed
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index b127ea1a0b38..bc1e3ee7ece8 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -2954,7 +2954,6 @@ rtl_verify_bb_layout (void)
 	    {
 	    case BARRIER:
 	    case NOTE:
-	    case DEBUG_INSN:
 	      break;
 
 	    case CODE_LABEL:


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Thu, Dec 14, 2017 at 04:08:45PM -0200, Alexandre Oliva wrote:
> On Dec 13, 2017, Alexandre Oliva <aoliva@redhat.com> wrote:
> 
> > On Dec 12, 2017, David Edelsohn <dje.gcc@gmail.com> wrote:
> >> Rainer,
> >> PR83396 opened.  you can add Solaris to the list of targets.
> 
> > Andreas,
> 
> > Here's a fix for the ia64 regression you mentioned in that PR.
> 
> And here's a patch that fixes the two other ia64 build failures you'd
> mentioned.
> 
> Regstrapped on x86_64- and i686-linux-gnu by myself; bootstrapped on
> ia64-linux-gnu by yourself IIUC, though with some weird bootstrap
> compare errors I'm very curious to learn more about.
> 
> Ok to install?
> 
> Emitting markers before labels turned out to not be worth the trouble.
> The markers outside BBs confuse the ebb scheduler, and they don't add
> any useful information.  I'll arrange for markers to be moved past
> labels, even in gimple, but for now this will fix the two remaining
> known problems on ia64.
> 
> for  gcc/ChangeLog
> 
> 	PR bootstrap/83396
> 	* cfgexpand.c (expand_gimple_basic_block): Expand label first,
> 	even if there are markers before it.
> 	* cfgrtl.c (rtl_verify_bb_layout): Reject DEBUG_INSNs outside BBs.

Ok, but please work on reversion of everything that has been changed
in order to support those (on RTL e.g. the var-tracking.c
      for (insn = get_first_insn (bb);
           insn != BB_HEAD (bb->next_bb)
             ? next = NEXT_INSN (insn), true : false;
           insn = next)
and
          rtx_insn *next;
          bool outside_bb = true;
          for (insn = get_first_insn (bb); insn != BB_HEAD (bb->next_bb);
               insn = next)
            {
              if (insn == BB_HEAD (bb))
                outside_bb = false;
              else if (insn == NEXT_INSN (BB_END (bb)))
                outside_bb = true;
...
                  if (outside_bb)
                    {
                      /* Ignore non-debug insns outside of basic blocks.  */
                      if (!DEBUG_INSN_P (insn))
                        continue;
                      /* Debug binds shouldn't appear outside of bbs.  */
                      gcc_assert (!DEBUG_BIND_INSN_P (insn));
                    }
and the NULL BLOCK_FOR_INSN stuff, on GIMPLE the gsi_after_labels changes,
the tree-cfg.c verification needs to be changed to disallow even the
markers before labels, ...).  That can be done incrementally, it is better
to unbreak the tree ASAP.

	Jakub

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 14, 2017, Alexandre Oliva <aoliva@redhat.com> wrote:

> I'll arrange for markers to be moved past labels, even in gimple

Here's a patch that implements this, and reverts all the changes I could
find that had been introduced to support debug markers before labels and
between BBs.

I have *not* fully tested it yet, nor do I intend to install it shortly;
at the earliest (assuming it's reviewed and approved) Monday or Tuesday
next week.

This will give me time to give it more varied testing (multiple
optimization flags, for one; more targets, if others volunteer testing,
or I can locate available machines in the GCC Compile Farm or elsewhere)

Assuming testing succeeds, is this ok to install?


[SFN] debug markers before labels no more

Make sure that gimple and RTL IRs don't have debug markers before
labels.  When we build the CFG, we move labels before any markers
appearing before them.  Then, make sure we don't mistakenly
reintroduce them.

This reverts some of the complexity that had been brought about by the
initial SFN patches.

for  gcc/ChangeLog

	PR bootstrap/83396
	* cfgexpand.c (label_rtx_for_bb): Revert SFN changes that
	allowed debug stmts before labels.
	(expand_gimple_basic_block): Likewise.
	* gimple-iterator.c (gimple_find_edge_insert_loc): Likewise.
	* gimple-iterator.h (gsi_after_labels): Likewise.
	* tree-cfgcleanup (remove_forwarder_block): Likewise, but
	rename reused variable, and simplify using gsi_move_before.
	* tree-ssa-tail-merge.c (find_duplicate): Likewise.
	* tree-cfg.c (make_blocks_1, make_edges): Likewise.
	(cleanup_dead_labels, gimple_can_merge_blocks_p): Likewise.
	(stmt_starts_bb_p, verify_gimple_in_cfg): Likewise.
	(gimple_verify_flow_info, gimple_block_label): Likewise.
	(make_blocks): Move debug markers after adjacent labels.
	* cfgrtl.c (skip_insns_after_block): Revert SFN changes that
	allowed debug insns outside blocks.
	* df-scan.c (df_insn_delete): Likewise.
	* lra-constraints.c (update_ebb_live_info): Likewise.
	* var-tracking.c (get_first_insn, vt_emit_notes): Likewise.
	(vt_initialize, delete_vta_debug_insns): Likewise.
	(reemit_marker_as_note): Drop BB parm.  Adjust callers.
---
 gcc/cfgexpand.c           |   13 +----
 gcc/cfgrtl.c              |    3 -
 gcc/df-scan.c             |    2 -
 gcc/gimple-iterator.c     |    8 +--
 gcc/gimple-iterator.h     |   15 +----
 gcc/lra-constraints.c     |    7 ---
 gcc/tree-cfg.c            |  123 ++++++++++++++++++++++++---------------------
 gcc/tree-cfgcleanup.c     |   49 ++++++++----------
 gcc/tree-ssa-tail-merge.c |    4 +
 gcc/var-tracking.c        |   65 ++----------------------
 10 files changed, 104 insertions(+), 185 deletions(-)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 30d9bac1118e..9c97f6e55691 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2327,9 +2327,6 @@ label_rtx_for_bb (basic_block bb ATTRIBUTE_UNUSED)
     {
       glabel *lab_stmt;
 
-      if (is_gimple_debug (gsi_stmt (gsi)))
-	continue;
-
       lab_stmt = dyn_cast <glabel *> (gsi_stmt (gsi));
       if (!lab_stmt)
 	break;
@@ -5498,16 +5495,14 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
 	}
     }
 
-  gsi = gsi_start_nondebug (stmts);
+  gsi = gsi_start (stmts);
   if (!gsi_end_p (gsi))
     {
       stmt = gsi_stmt (gsi);
       if (gimple_code (stmt) != GIMPLE_LABEL)
 	stmt = NULL;
     }
-  gsi = gsi_start (stmts);
 
-  gimple *label_stmt = stmt;
   rtx_code_label **elt = lab_rtx_for_bb->get (bb);
 
   if (stmt || elt)
@@ -5518,8 +5513,7 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
       if (stmt)
 	{
 	  expand_gimple_stmt (stmt);
-	  if (gsi_stmt (gsi) == stmt)
-	    gsi_next (&gsi);
+	  gsi_next (&gsi);
 	}
 
       if (elt)
@@ -5545,9 +5539,6 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
 
       stmt = gsi_stmt (gsi);
 
-      if (stmt == label_stmt)
-	continue;
-
       /* If this statement is a non-debug one, and we generate debug
 	 insns, then this one might be the last real use of a TERed
 	 SSA_NAME, but where there are still some debug uses further
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index bc1e3ee7ece8..e4cdab533a49 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -3390,9 +3390,6 @@ skip_insns_after_block (basic_block bb)
 	  last_insn = insn;
 	  continue;
 
-	case DEBUG_INSN:
-	  continue;
-
 	case NOTE:
 	  switch (NOTE_KIND (insn))
 	    {
diff --git a/gcc/df-scan.c b/gcc/df-scan.c
index 429dab8a99b4..8ab3d716ea29 100644
--- a/gcc/df-scan.c
+++ b/gcc/df-scan.c
@@ -945,7 +945,7 @@ df_insn_delete (rtx_insn *insn)
      In any case, we expect BB to be non-NULL at least up to register
      allocation, so disallow a non-NULL BB up to there.  Not perfect
      but better than nothing...  */
-  gcc_checking_assert (bb != NULL || DEBUG_INSN_P (insn) || reload_completed);
+  gcc_checking_assert (bb != NULL || reload_completed);
 
   df_grow_bb_info (df_scan);
   df_grow_reg_info ();
diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c
index 46d3c6ab6928..258bbee7ffce 100644
--- a/gcc/gimple-iterator.c
+++ b/gcc/gimple-iterator.c
@@ -743,13 +743,9 @@ gimple_find_edge_insert_loc (edge e, gimple_stmt_iterator *gsi,
       if (gsi_end_p (*gsi))
 	return true;
 
-      /* Make sure we insert after any leading labels.  We have to
-	 skip debug stmts before or among them, though.  We didn't
-	 have to skip debug stmts after the last label, but it
-	 shouldn't hurt if we do.  */
+      /* Make sure we insert after any leading labels.  */
       tmp = gsi_stmt (*gsi);
-      while (gimple_code (tmp) == GIMPLE_LABEL
-	     || is_gimple_debug (tmp))
+      while (gimple_code (tmp) == GIMPLE_LABEL)
 	{
 	  gsi_next (gsi);
 	  if (gsi_end_p (*gsi))
diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h
index 167edc18db5b..e655ef857fa8 100644
--- a/gcc/gimple-iterator.h
+++ b/gcc/gimple-iterator.h
@@ -213,24 +213,17 @@ gsi_stmt (gimple_stmt_iterator i)
 }
 
 /* Return a block statement iterator that points to the first
-   non-label statement in block BB.  Skip debug stmts only if they
-   precede labels.  */
+   non-label statement in block BB.  */
 
 static inline gimple_stmt_iterator
 gsi_after_labels (basic_block bb)
 {
   gimple_stmt_iterator gsi = gsi_start_bb (bb);
 
-  for (gimple_stmt_iterator gskip = gsi;
-       !gsi_end_p (gskip); )
+  for (; !gsi_end_p (gsi); )
     {
-      if (is_gimple_debug (gsi_stmt (gskip)))
-	gsi_next (&gskip);
-      else if (gimple_code (gsi_stmt (gskip)) == GIMPLE_LABEL)
-	{
-	  gsi_next (&gskip);
-	  gsi = gskip;
-	}
+      if (gimple_code (gsi_stmt (gsi)) == GIMPLE_LABEL)
+	gsi_next (&gsi);
       else
 	break;
     }
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 4adf4bfea8b0..24bfc86fc88e 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -5808,13 +5808,6 @@ update_ebb_live_info (rtx_insn *head, rtx_insn *tail)
       if (NOTE_P (curr_insn) && NOTE_KIND (curr_insn) != NOTE_INSN_BASIC_BLOCK)
 	continue;
       curr_bb = BLOCK_FOR_INSN (curr_insn);
-      if (!curr_bb)
-	{
-	  gcc_assert (DEBUG_INSN_P (curr_insn));
-	  if (DEBUG_MARKER_INSN_P (curr_insn))
-	    continue;
-	  curr_bb = prev_bb;
-	}
       if (curr_bb != prev_bb)
 	{
 	  if (prev_bb != NULL)
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 75a0a302e96f..4ec356d204d1 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -561,22 +561,14 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
 {
   gimple_stmt_iterator i = gsi_start (seq);
   gimple *stmt = NULL;
-  gimple *prev_stmt = NULL;
   bool start_new_block = true;
   bool first_stmt_of_seq = true;
 
   while (!gsi_end_p (i))
     {
-      /* PREV_STMT should only be set to a debug stmt if the debug
-	 stmt is before nondebug stmts.  Once stmt reaches a nondebug
-	 nonlabel, prev_stmt will be set to it, so that
-	 stmt_starts_bb_p will know to start a new block if a label is
-	 found.  However, if stmt was a label after debug stmts only,
-	 keep the label in prev_stmt even if we find further debug
-	 stmts, for there may be other labels after them, and they
-	 should land in the same block.  */
-      if (!prev_stmt || !stmt || !is_gimple_debug (stmt))
-	prev_stmt = stmt;
+      gimple *prev_stmt;
+
+      prev_stmt = stmt;
       stmt = gsi_stmt (i);
 
       if (stmt && is_gimple_call (stmt))
@@ -591,7 +583,6 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
 	    gsi_split_seq_before (&i, &seq);
 	  bb = create_basic_block (seq, bb);
 	  start_new_block = false;
-	  prev_stmt = NULL;
 	}
 
       /* Now add STMT to BB and create the subgraphs for special statement
@@ -636,6 +627,67 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
 static void
 make_blocks (gimple_seq seq)
 {
+  /* Look for debug markers right before labels, and move the debug
+     stmts after the labels.  Accepting labels among debug markers
+     adds no value, just complexity; if we wanted to annotate labels
+     with view numbers (so sequencing among markers would matter) or
+     somesuch, we're probably better off still moving the labels, but
+     adding other debug annotations in their original positions or
+     emitting nonbind or bind markers associated with the labels in
+     the original position of the labels.
+
+     Moving labels would probably be simpler, but we can't do that:
+     moving labels assigns label ids to them, and doing so because of
+     debug markers makes for -fcompare-debug and possibly even codegen
+     differences.  So, we have to move the debug stmts instead.  To
+     that end, we scan SEQ backwards, marking the position of the
+     latest (earliest we find) label, and moving debug stmts that are
+     not separated from it by nondebug nonlabel stmts after the
+     label.  */
+  if (MAY_HAVE_DEBUG_MARKER_STMTS)
+    {
+      gimple_stmt_iterator label = gsi_none ();
+
+      for (gimple_stmt_iterator i = gsi_last (seq); !gsi_end_p (i); gsi_prev (&i))
+	{
+	  gimple *stmt = gsi_stmt (i);
+
+	  /* If this is the first label we encounter (latest in SEQ)
+	     before nondebug stmts, record its position.  */
+	  if (is_a <glabel *> (stmt))
+	    {
+	      if (gsi_end_p (label))
+		label = i;
+	      continue;
+	    }
+
+	  /* Without a recorded label position to move debug stmts to,
+	     there's nothing to do.  */
+	  if (gsi_end_p (label))
+	    continue;
+
+	  /* Move the debug stmt at I after LABEL.  */
+	  if (is_gimple_debug (stmt))
+	    {
+	      gcc_assert (gimple_debug_nonbind_marker_p (stmt));
+	      /* As STMT is removed, I advances to the stmt after
+		 STMT, so the gsi_prev in the for "increment"
+		 expression gets us to the stmt we're to visit after
+		 STMT.  LABEL, however, would advance to the moved
+		 stmt if we passed it to gsi_move_after, so pass it a
+		 copy instead, so as to keep LABEL pointing to the
+		 LABEL.  */
+	      gimple_stmt_iterator copy = label;
+	      gsi_move_after (&i, &copy);
+	      continue;
+	    }
+
+	  /* There aren't any (more?) debug stmts before label, so
+	     there isn't anything else to move after it.  */
+	  label = gsi_none ();
+	}
+    }
+
   make_blocks_1 (seq, ENTRY_BLOCK_PTR_FOR_FN (cfun));
 }
 
@@ -1005,11 +1057,7 @@ make_edges (void)
 	      tree target;
 
 	      if (!label_stmt)
-		{
-		  if (is_gimple_debug (gsi_stmt (gsi)))
-		    continue;
-		  break;
-		}
+		break;
 
 	      target = gimple_label_label (label_stmt);
 
@@ -1519,9 +1567,6 @@ cleanup_dead_labels (void)
 
       for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
 	{
-	  if (is_gimple_debug (gsi_stmt (i)))
-	    continue;
-
 	  tree label;
 	  glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (i));
 
@@ -1682,12 +1727,6 @@ cleanup_dead_labels (void)
 
       for (i = gsi_start_bb (bb); !gsi_end_p (i); )
 	{
-	  if (is_gimple_debug (gsi_stmt (i)))
-	    {
-	      gsi_next (&i);
-	      continue;
-	    }
-
 	  tree label;
 	  glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (i));
 
@@ -1863,8 +1902,6 @@ gimple_can_merge_blocks_p (basic_block a, basic_block b)
        gsi_next (&gsi))
     {
       tree lab;
-      if (is_gimple_debug (gsi_stmt (gsi)))
-	continue;
       glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (gsi));
       if (!label_stmt)
 	break;
@@ -2666,13 +2703,6 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
   if (stmt == NULL)
     return false;
 
-  /* PREV_STMT is only set to a debug stmt if the debug stmt is before
-     any nondebug stmts in the block.  We don't want to start another
-     block in this case: the debug stmt will already have started the
-     one STMT would start if we weren't outputting debug stmts.  */
-  if (prev_stmt && is_gimple_debug (prev_stmt))
-    return false;
-
   /* Labels start a new basic block only if the preceding statement
      wasn't a label of the same type.  This prevents the creation of
      consecutive blocks that have nothing but a single label.  */
@@ -5380,7 +5410,6 @@ verify_gimple_in_cfg (struct function *fn, bool verify_nothrow)
 	  err |= err2;
 	}
 
-      bool label_allowed = true;
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  gimple *stmt = gsi_stmt (gsi);
@@ -5397,19 +5426,6 @@ verify_gimple_in_cfg (struct function *fn, bool verify_nothrow)
 	      err2 = true;
 	    }
 
-	  /* Labels may be preceded only by debug markers, not debug bind
-	     or source bind or any other statements.  */
-	  if (gimple_code (stmt) == GIMPLE_LABEL)
-	    {
-	      if (!label_allowed)
-		{
-		  error ("gimple label in the middle of a basic block");
-		  err2 = true;
-		}
-	    }
-	  else if (!gimple_debug_begin_stmt_p (stmt))
-	    label_allowed = false;
-
 	  err2 |= verify_gimple_stmt (stmt);
 	  err2 |= verify_location (&blocks, gimple_location (stmt));
 
@@ -5533,10 +5549,6 @@ gimple_verify_flow_info (void)
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  tree label;
-
-	  if (is_gimple_debug (gsi_stmt (gsi)))
-	    continue;
-
 	  gimple *prev_stmt = stmt;
 
 	  stmt = gsi_stmt (gsi);
@@ -5861,10 +5873,8 @@ gimple_block_label (basic_block bb)
   tree label;
   glabel *stmt;
 
-  for (i = s; !gsi_end_p (i); gsi_next (&i))
+  for (i = s; !gsi_end_p (i); first = false, gsi_next (&i))
     {
-      if (is_gimple_debug (gsi_stmt (i)))
-	continue;
       stmt = dyn_cast <glabel *> (gsi_stmt (i));
       if (!stmt)
 	break;
@@ -5875,7 +5885,6 @@ gimple_block_label (basic_block bb)
 	    gsi_move_before (&i, &s);
 	  return label;
 	}
-      first = false;
     }
 
   label = create_artificial_label (UNKNOWN_LOCATION);
diff --git a/gcc/tree-cfgcleanup.c b/gcc/tree-cfgcleanup.c
index a0e5797ec0ec..bfcca03257d3 100644
--- a/gcc/tree-cfgcleanup.c
+++ b/gcc/tree-cfgcleanup.c
@@ -444,7 +444,7 @@ remove_forwarder_block (basic_block bb)
 {
   edge succ = single_succ_edge (bb), e, s;
   basic_block dest = succ->dest;
-  gimple *label;
+  gimple *stmt;
   edge_iterator ei;
   gimple_stmt_iterator gsi, gsi_to;
   bool can_move_debug_stmts;
@@ -457,9 +457,9 @@ remove_forwarder_block (basic_block bb)
 
   /* If the destination block consists of a nonlocal label or is a
      EH landing pad, do not merge it.  */
-  label = first_stmt (dest);
-  if (label)
-    if (glabel *label_stmt = dyn_cast <glabel *> (label))
+  stmt = first_stmt (dest);
+  if (stmt)
+    if (glabel *label_stmt = dyn_cast <glabel *> (stmt))
       if (DECL_NONLOCAL (gimple_label_label (label_stmt))
 	  || EH_LANDING_PAD_NR (gimple_label_label (label_stmt)) != 0)
 	return false;
@@ -536,28 +536,23 @@ remove_forwarder_block (basic_block bb)
      defined labels and labels with an EH landing pad number to the
      new block, so that the redirection of the abnormal edges works,
      jump targets end up in a sane place and debug information for
-     labels is retained.
-
-     While at that, move any debug stmts that appear before or in between
-     labels, but not those that can only appear after labels.  */
+     labels is retained.  */
   gsi_to = gsi_start_bb (dest);
-  gsi = gsi_start_bb (bb);
-  gimple_stmt_iterator gsie = gsi_after_labels (bb);
-  while (gsi_stmt (gsi) != gsi_stmt (gsie))
+  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
     {
-      tree decl;
-      label = gsi_stmt (gsi);
-      if (is_gimple_debug (label)
-	  ? can_move_debug_stmts
-	  : ((decl = gimple_label_label (as_a <glabel *> (label))),
-	     EH_LANDING_PAD_NR (decl) != 0
-	     || DECL_NONLOCAL (decl)
-	     || FORCED_LABEL (decl)
-	     || !DECL_ARTIFICIAL (decl)))
-	{
-	  gsi_remove (&gsi, false);
-	  gsi_insert_before (&gsi_to, label, GSI_SAME_STMT);
-	}
+      stmt = gsi_stmt (gsi);
+      if (is_gimple_debug (stmt))
+	break;
+
+      /* Forwarder blocks can only contain labels and debug stmts, and
+	 labels must come first, so if we get to this point, we know
+	 we're looking at a label.  */
+      tree decl = gimple_label_label (as_a <glabel *> (stmt));
+      if (EH_LANDING_PAD_NR (decl) != 0
+	  || DECL_NONLOCAL (decl)
+	  || FORCED_LABEL (decl)
+	  || !DECL_ARTIFICIAL (decl))
+	gsi_move_before (&gsi, &gsi_to);
       else
 	gsi_next (&gsi);
     }
@@ -565,14 +560,12 @@ remove_forwarder_block (basic_block bb)
   /* Move debug statements if the destination has a single predecessor.  */
   if (can_move_debug_stmts && !gsi_end_p (gsi))
     {
-      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
-      gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
+      gsi_to = gsi_after_labels (dest);
       do
 	{
 	  gimple *debug = gsi_stmt (gsi);
 	  gcc_assert (is_gimple_debug (debug));
-	  gsi_remove (&gsi, false);
-	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
+	  gsi_move_before (&gsi, &gsi_to);
 	}
       while (!gsi_end_p (gsi));
     }
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 743ee4e1cf99..fc94f5d83d2c 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -1295,14 +1295,14 @@ find_duplicate (same_succ *same_succ, basic_block bb1, basic_block bb2)
       tree label = gimple_label_label (as_a <glabel *> (gsi_stmt (gsi1)));
       if (DECL_NONLOCAL (label) || FORCED_LABEL (label))
 	return;
-      gsi_prev_nondebug (&gsi1);
+      gsi_prev (&gsi1);
     }
   while (!gsi_end_p (gsi2) && gimple_code (gsi_stmt (gsi2)) == GIMPLE_LABEL)
     {
       tree label = gimple_label_label (as_a <glabel *> (gsi_stmt (gsi2)));
       if (DECL_NONLOCAL (label) || FORCED_LABEL (label))
 	return;
-      gsi_prev_nondebug (&gsi2);
+      gsi_prev (&gsi2);
     }
   if (!(gsi_end_p (gsi1) && gsi_end_p (gsi2)))
     return;
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index a913855b67a5..3bdeb6c885f6 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -9479,24 +9479,6 @@ emit_notes_in_bb (basic_block bb, dataflow_set *set)
     }
 }
 
-/* Return BB's head, unless BB is the block that succeeds ENTRY_BLOCK,
-   in which case it searches back from BB's head for the very first
-   insn.  Use [get_first_insn (bb), BB_HEAD (bb->next_bb)[ as a range
-   to iterate over all insns of a function while iterating over its
-   BBs.  */
-
-static rtx_insn *
-get_first_insn (basic_block bb)
-{
-  rtx_insn *insn = BB_HEAD (bb);
-
-  if (bb->prev_bb == ENTRY_BLOCK_PTR_FOR_FN (cfun))
-    while (rtx_insn *prev = PREV_INSN (insn))
-      insn = prev;
-
-  return insn;
-}
-
 /* Emit notes for the whole function.  */
 
 static void
@@ -9525,8 +9507,7 @@ vt_emit_notes (void)
     {
       /* Emit the notes for changes of variable locations between two
 	 subsequent basic blocks.  */
-      emit_notes_for_differences (get_first_insn (bb),
-				  &cur, &VTI (bb)->in);
+      emit_notes_for_differences (BB_HEAD (bb), &cur, &VTI (bb)->in);
 
       if (MAY_HAVE_DEBUG_BIND_INSNS)
 	local_get_addr_cache = new hash_map<rtx, rtx>;
@@ -9929,7 +9910,7 @@ vt_init_cfa_base (void)
 /* Reemit INSN, a MARKER_DEBUG_INSN, as a note.  */
 
 static rtx_insn *
-reemit_marker_as_note (rtx_insn *insn, basic_block *bb)
+reemit_marker_as_note (rtx_insn *insn)
 {
   gcc_checking_assert (DEBUG_MARKER_INSN_P (insn));
 
@@ -9944,8 +9925,6 @@ reemit_marker_as_note (rtx_insn *insn, basic_block *bb)
 	  {
 	    note = emit_note_before (kind, insn);
 	    NOTE_MARKER_LOCATION (note) = INSN_LOCATION (insn);
-	    if (bb)
-	      BLOCK_FOR_INSN (note) = *bb;
 	  }
 	delete_insn (insn);
 	return note;
@@ -10153,39 +10132,11 @@ vt_initialize (void)
 	  HOST_WIDE_INT offset = VTI (bb)->out.stack_adjust;
 	  VTI (bb)->out.stack_adjust = VTI (bb)->in.stack_adjust;
 
-	  /* If we are walking the first basic block, walk any HEADER
-	     insns that might be before it too.  Unfortunately,
-	     BB_HEADER and BB_FOOTER are not set while we run this
-	     pass.  */
 	  rtx_insn *next;
-	  bool outside_bb = true;
-	  for (insn = get_first_insn (bb); insn != BB_HEAD (bb->next_bb);
-	       insn = next)
+	  FOR_BB_INSNS_SAFE (bb, insn, next)
 	    {
-	      if (insn == BB_HEAD (bb))
-		outside_bb = false;
-	      else if (insn == NEXT_INSN (BB_END (bb)))
-		outside_bb = true;
-	      next = NEXT_INSN (insn);
 	      if (INSN_P (insn))
 		{
-		  if (outside_bb)
-		    {
-		      /* Ignore non-debug insns outside of basic blocks.  */
-		      if (!DEBUG_INSN_P (insn))
-			continue;
-		      /* Debug binds shouldn't appear outside of bbs.  */
-		      gcc_assert (!DEBUG_BIND_INSN_P (insn));
-		    }
-		  basic_block save_bb = BLOCK_FOR_INSN (insn);
-		  if (!BLOCK_FOR_INSN (insn))
-		    {
-		      gcc_assert (outside_bb);
-		      BLOCK_FOR_INSN (insn) = bb;
-		    }
-		  else
-		    gcc_assert (BLOCK_FOR_INSN (insn) == bb);
-
 		  if (!frame_pointer_needed)
 		    {
 		      insn_stack_adjust_offset_pre_post (insn, &pre, &post);
@@ -10207,7 +10158,7 @@ vt_initialize (void)
 		  adjust_insn (bb, insn);
 		  if (DEBUG_MARKER_INSN_P (insn))
 		    {
-		      insn = reemit_marker_as_note (insn, &save_bb);
+		      reemit_marker_as_note (insn);
 		      continue;
 		    }
 
@@ -10259,7 +10210,6 @@ vt_initialize (void)
 			    }
 			}
 		    }
-		  BLOCK_FOR_INSN (insn) = save_bb;
 		}
 	    }
 	  gcc_assert (offset == VTI (bb)->out.stack_adjust);
@@ -10301,15 +10251,12 @@ delete_vta_debug_insns (void)
 
   FOR_EACH_BB_FN (bb, cfun)
     {
-      for (insn = get_first_insn (bb);
-	   insn != BB_HEAD (bb->next_bb)
-	     ? next = NEXT_INSN (insn), true : false;
-	   insn = next)
+      FOR_BB_INSNS_SAFE (bb, insn, next)
 	if (DEBUG_INSN_P (insn))
 	  {
 	    if (DEBUG_MARKER_INSN_P (insn))
 	      {
-		insn = reemit_marker_as_note (insn, NULL);
+		reemit_marker_as_note (insn);
 		continue;
 	      }
 


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 14, 2017, Alexandre Oliva <oliva@gnu.org> wrote:

> On Dec 14, 2017, Alexandre Oliva <aoliva@redhat.com> wrote:
>> I'll arrange for markers to be moved past labels, even in gimple

> Here's a patch that implements this, and reverts all the changes I could
> find that had been introduced to support debug markers before labels and
> between BBs.

> I have *not* fully tested it yet

I reverted too much :-(  see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83396#c77 for details, or
just apply this patchlet on top of the earlier one (it reinstates some
of the SFN changes that turned out to be needed for more than accepting
markers before labels)


diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 4ec356d204d1..7fe0a1eec4c8 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -561,14 +561,22 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
 {
   gimple_stmt_iterator i = gsi_start (seq);
   gimple *stmt = NULL;
+  gimple *prev_stmt = NULL;
   bool start_new_block = true;
   bool first_stmt_of_seq = true;
 
   while (!gsi_end_p (i))
     {
-      gimple *prev_stmt;
-
-      prev_stmt = stmt;
+      /* PREV_STMT should only be set to a debug stmt if the debug
+	 stmt is before nondebug stmts.  Once stmt reaches a nondebug
+	 nonlabel, prev_stmt will be set to it, so that
+	 stmt_starts_bb_p will know to start a new block if a label is
+	 found.  However, if stmt was a label after debug stmts only,
+	 keep the label in prev_stmt even if we find further debug
+	 stmts, for there may be other labels after them, and they
+	 should land in the same block.  */
+      if (!prev_stmt || !stmt || !is_gimple_debug (stmt))
+	prev_stmt = stmt;
       stmt = gsi_stmt (i);
 
       if (stmt && is_gimple_call (stmt))
@@ -583,6 +591,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
 	    gsi_split_seq_before (&i, &seq);
 	  bb = create_basic_block (seq, bb);
 	  start_new_block = false;
+	  prev_stmt = NULL;
 	}
 
       /* Now add STMT to BB and create the subgraphs for special statement
@@ -2703,6 +2712,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
   if (stmt == NULL)
     return false;
 
+  /* PREV_STMT is only set to a debug stmt if the debug stmt is before
+     any nondebug stmts in the block.  We don't want to start another
+     block in this case: the debug stmt will already have started the
+     one STMT would start if we weren't outputting debug stmts.  */
+  if (prev_stmt && is_gimple_debug (prev_stmt))
+    return false;
+
   /* Labels start a new basic block only if the preceding statement
      wasn't a label of the same type.  This prevents the creation of
      consecutive blocks that have nothing but a single label.  */

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

Hi!

I'll try to read it in more details later today, but one thing I've noticed:

On Thu, Dec 14, 2017 at 11:51:29PM -0200, Alexandre Oliva wrote:
> @@ -5380,7 +5410,6 @@ verify_gimple_in_cfg (struct function *fn, bool verify_nothrow)
>  	  err |= err2;
>  	}
>  
> -      bool label_allowed = true;
>        for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>  	{
>  	  gimple *stmt = gsi_stmt (gsi);
> @@ -5397,19 +5426,6 @@ verify_gimple_in_cfg (struct function *fn, bool verify_nothrow)
>  	      err2 = true;
>  	    }
>  
> -	  /* Labels may be preceded only by debug markers, not debug bind
> -	     or source bind or any other statements.  */
> -	  if (gimple_code (stmt) == GIMPLE_LABEL)
> -	    {
> -	      if (!label_allowed)
> -		{
> -		  error ("gimple label in the middle of a basic block");
> -		  err2 = true;
> -		}
> -	    }
> -	  else if (!gimple_debug_begin_stmt_p (stmt))
> -	    label_allowed = false;
> -

Please don't revert the above 2 hunks.  Instead just remove the
> -       else if (!gimple_debug_begin_stmt_p (stmt))
> -         label_allowed = false;
lines only from it and adjust the comment.  We want to verify there are
no statements before labels.

	Jakub

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 15, 2017, Jakub Jelinek <jakub@redhat.com> wrote:

> Please don't revert the above 2 hunks.  [...]  We want to verify there are
> no statements before labels.

Why, sure!  Thanks for catching this.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 15, 2017, Jakub Jelinek <jakub@redhat.com> wrote:

> We want to verify there are no statements before labels.

Erhm, actually...

We already verify that in gimple_verify_flow_info.

Is there any point in having such redundant (?) verification?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Tue, Dec 19, 2017 at 04:29:53PM -0200, Alexandre Oliva wrote:
> On Dec 15, 2017, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > We want to verify there are no statements before labels.
> 
> Erhm, actually...
> 
> We already verify that in gimple_verify_flow_info.
> 
> Is there any point in having such redundant (?) verification?

Ah, you're right.  The reason I've added was that you've added
          if (is_gimple_debug (gsi_stmt (gsi)))
            continue;
there and so it didn't catch those.  But now you're reverting that
and so it should be effective again.

Has the patch you've posted passed bootstrap/regtest?
I'll try to eyeball it in detail tomorrow (or if you have spare cycles,
just try to verify it against revision before SFNs were introduced)
that the affected spots are reverted to the old code unless needed for SFNs
without markers in between bbs.

	Jakub

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 19, 2017, Jakub Jelinek <jakub@redhat.com> wrote:

> Has the patch you've posted passed bootstrap/regtest?

Yeah, here's the latest version, that survived O1, O2 and O3 bootstraps
with bootstrap-debug (-g0 for stage2), bootstrap-debug-lean
(-fcompare-debug for stage3) and bootstrap-debug-lib (-fcompare-debug
for target libs) on all of x86_64-, i686-, ppc64-, ppc64le-, and
aarch64-, and also survived a regular bootstrap on sparc64-linux-gnu (it
had a few -fcompare-debug failures that I'm yet to investigate, though).

Ok to install?


> I'll try to eyeball it in detail tomorrow (or if you have spare cycles,
> just try to verify it against revision before SFNs were introduced)
> that the affected spots are reverted to the old code unless needed for SFNs
> without markers in between bbs.

I reviewed the entire SFN patchset looking for such spots, and reverted
all the bits that seemed relevant; that's how I got to the patch below.
I went too far with the vfork-breaking reversal, but after putting that
back in, testing proceeded smoothly.

BTW, we don't want markers in between BBs either.



[SFN] debug markers before labels no more

Make sure that gimple and RTL IRs don't have debug markers before
labels.  When we build the CFG, we move labels before any markers
appearing before them.  Then, make sure we don't mistakenly
reintroduce them.

This reverts some of the complexity that had been brought about by the
initial SFN patches.

for  gcc/ChangeLog

	PR bootstrap/83396
	* cfgexpand.c (label_rtx_for_bb): Revert SFN changes that
	allowed debug stmts before labels.
	(expand_gimple_basic_block): Likewise.
	* gimple-iterator.c (gimple_find_edge_insert_loc): Likewise.
	* gimple-iterator.h (gsi_after_labels): Likewise.
	* tree-cfgcleanup (remove_forwarder_block): Likewise, but
	rename reused variable, and simplify using gsi_move_before.
	* tree-ssa-tail-merge.c (find_duplicate): Likewise.
	* tree-cfg.c (make_edges, cleanup_dead_labels): Likewise.
	(gimple_can_merge_blocks_p, verify_gimple_in_cfg): Likewise.
	(gimple_verify_flow_info, gimple_block_label): Likewise.
	(make_blocks): Move debug markers after adjacent labels.
	* cfgrtl.c (skip_insns_after_block): Revert SFN changes that
	allowed debug insns outside blocks.
	* df-scan.c (df_insn_delete): Likewise.
	* lra-constraints.c (update_ebb_live_info): Likewise.
	* var-tracking.c (get_first_insn, vt_emit_notes): Likewise.
	(vt_initialize, delete_vta_debug_insns): Likewise.
	(reemit_marker_as_note): Drop BB parm.  Adjust callers.
---
 gcc/cfgexpand.c           |   13 +-----
 gcc/cfgrtl.c              |    3 -
 gcc/df-scan.c             |    2 -
 gcc/gimple-iterator.c     |    8 +---
 gcc/gimple-iterator.h     |   15 ++-----
 gcc/lra-constraints.c     |    7 ---
 gcc/tree-cfg.c            |  101 ++++++++++++++++++++++++++++-----------------
 gcc/tree-cfgcleanup.c     |   49 +++++++++-------------
 gcc/tree-ssa-tail-merge.c |    4 +-
 gcc/var-tracking.c        |   65 +++--------------------------
 10 files changed, 101 insertions(+), 166 deletions(-)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d1616e192cb5..186012b0360f 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2327,9 +2327,6 @@ label_rtx_for_bb (basic_block bb ATTRIBUTE_UNUSED)
     {
       glabel *lab_stmt;
 
-      if (is_gimple_debug (gsi_stmt (gsi)))
-	continue;
-
       lab_stmt = dyn_cast <glabel *> (gsi_stmt (gsi));
       if (!lab_stmt)
 	break;
@@ -5503,16 +5500,14 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
 	}
     }
 
-  gsi = gsi_start_nondebug (stmts);
+  gsi = gsi_start (stmts);
   if (!gsi_end_p (gsi))
     {
       stmt = gsi_stmt (gsi);
       if (gimple_code (stmt) != GIMPLE_LABEL)
 	stmt = NULL;
     }
-  gsi = gsi_start (stmts);
 
-  gimple *label_stmt = stmt;
   rtx_code_label **elt = lab_rtx_for_bb->get (bb);
 
   if (stmt || elt)
@@ -5523,8 +5518,7 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
       if (stmt)
 	{
 	  expand_gimple_stmt (stmt);
-	  if (gsi_stmt (gsi) == stmt)
-	    gsi_next (&gsi);
+	  gsi_next (&gsi);
 	}
 
       if (elt)
@@ -5550,9 +5544,6 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
 
       stmt = gsi_stmt (gsi);
 
-      if (stmt == label_stmt)
-	continue;
-
       /* If this statement is a non-debug one, and we generate debug
 	 insns, then this one might be the last real use of a TERed
 	 SSA_NAME, but where there are still some debug uses further
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index 45cccba680c9..e2da7608a91b 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -3390,9 +3390,6 @@ skip_insns_after_block (basic_block bb)
 	  last_insn = insn;
 	  continue;
 
-	case DEBUG_INSN:
-	  continue;
-
 	case NOTE:
 	  switch (NOTE_KIND (insn))
 	    {
diff --git a/gcc/df-scan.c b/gcc/df-scan.c
index 429dab8a99b4..8ab3d716ea29 100644
--- a/gcc/df-scan.c
+++ b/gcc/df-scan.c
@@ -945,7 +945,7 @@ df_insn_delete (rtx_insn *insn)
      In any case, we expect BB to be non-NULL at least up to register
      allocation, so disallow a non-NULL BB up to there.  Not perfect
      but better than nothing...  */
-  gcc_checking_assert (bb != NULL || DEBUG_INSN_P (insn) || reload_completed);
+  gcc_checking_assert (bb != NULL || reload_completed);
 
   df_grow_bb_info (df_scan);
   df_grow_reg_info ();
diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c
index 46d3c6ab6928..258bbee7ffce 100644
--- a/gcc/gimple-iterator.c
+++ b/gcc/gimple-iterator.c
@@ -743,13 +743,9 @@ gimple_find_edge_insert_loc (edge e, gimple_stmt_iterator *gsi,
       if (gsi_end_p (*gsi))
 	return true;
 
-      /* Make sure we insert after any leading labels.  We have to
-	 skip debug stmts before or among them, though.  We didn't
-	 have to skip debug stmts after the last label, but it
-	 shouldn't hurt if we do.  */
+      /* Make sure we insert after any leading labels.  */
       tmp = gsi_stmt (*gsi);
-      while (gimple_code (tmp) == GIMPLE_LABEL
-	     || is_gimple_debug (tmp))
+      while (gimple_code (tmp) == GIMPLE_LABEL)
 	{
 	  gsi_next (gsi);
 	  if (gsi_end_p (*gsi))
diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h
index 167edc18db5b..e655ef857fa8 100644
--- a/gcc/gimple-iterator.h
+++ b/gcc/gimple-iterator.h
@@ -213,24 +213,17 @@ gsi_stmt (gimple_stmt_iterator i)
 }
 
 /* Return a block statement iterator that points to the first
-   non-label statement in block BB.  Skip debug stmts only if they
-   precede labels.  */
+   non-label statement in block BB.  */
 
 static inline gimple_stmt_iterator
 gsi_after_labels (basic_block bb)
 {
   gimple_stmt_iterator gsi = gsi_start_bb (bb);
 
-  for (gimple_stmt_iterator gskip = gsi;
-       !gsi_end_p (gskip); )
+  for (; !gsi_end_p (gsi); )
     {
-      if (is_gimple_debug (gsi_stmt (gskip)))
-	gsi_next (&gskip);
-      else if (gimple_code (gsi_stmt (gskip)) == GIMPLE_LABEL)
-	{
-	  gsi_next (&gskip);
-	  gsi = gskip;
-	}
+      if (gimple_code (gsi_stmt (gsi)) == GIMPLE_LABEL)
+	gsi_next (&gsi);
       else
 	break;
     }
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 4adf4bfea8b0..24bfc86fc88e 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -5808,13 +5808,6 @@ update_ebb_live_info (rtx_insn *head, rtx_insn *tail)
       if (NOTE_P (curr_insn) && NOTE_KIND (curr_insn) != NOTE_INSN_BASIC_BLOCK)
 	continue;
       curr_bb = BLOCK_FOR_INSN (curr_insn);
-      if (!curr_bb)
-	{
-	  gcc_assert (DEBUG_INSN_P (curr_insn));
-	  if (DEBUG_MARKER_INSN_P (curr_insn))
-	    continue;
-	  curr_bb = prev_bb;
-	}
       if (curr_bb != prev_bb)
 	{
 	  if (prev_bb != NULL)
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 4bf621895cd6..e1d96b05fb3a 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -636,6 +636,67 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
 static void
 make_blocks (gimple_seq seq)
 {
+  /* Look for debug markers right before labels, and move the debug
+     stmts after the labels.  Accepting labels among debug markers
+     adds no value, just complexity; if we wanted to annotate labels
+     with view numbers (so sequencing among markers would matter) or
+     somesuch, we're probably better off still moving the labels, but
+     adding other debug annotations in their original positions or
+     emitting nonbind or bind markers associated with the labels in
+     the original position of the labels.
+
+     Moving labels would probably be simpler, but we can't do that:
+     moving labels assigns label ids to them, and doing so because of
+     debug markers makes for -fcompare-debug and possibly even codegen
+     differences.  So, we have to move the debug stmts instead.  To
+     that end, we scan SEQ backwards, marking the position of the
+     latest (earliest we find) label, and moving debug stmts that are
+     not separated from it by nondebug nonlabel stmts after the
+     label.  */
+  if (MAY_HAVE_DEBUG_MARKER_STMTS)
+    {
+      gimple_stmt_iterator label = gsi_none ();
+
+      for (gimple_stmt_iterator i = gsi_last (seq); !gsi_end_p (i); gsi_prev (&i))
+	{
+	  gimple *stmt = gsi_stmt (i);
+
+	  /* If this is the first label we encounter (latest in SEQ)
+	     before nondebug stmts, record its position.  */
+	  if (is_a <glabel *> (stmt))
+	    {
+	      if (gsi_end_p (label))
+		label = i;
+	      continue;
+	    }
+
+	  /* Without a recorded label position to move debug stmts to,
+	     there's nothing to do.  */
+	  if (gsi_end_p (label))
+	    continue;
+
+	  /* Move the debug stmt at I after LABEL.  */
+	  if (is_gimple_debug (stmt))
+	    {
+	      gcc_assert (gimple_debug_nonbind_marker_p (stmt));
+	      /* As STMT is removed, I advances to the stmt after
+		 STMT, so the gsi_prev in the for "increment"
+		 expression gets us to the stmt we're to visit after
+		 STMT.  LABEL, however, would advance to the moved
+		 stmt if we passed it to gsi_move_after, so pass it a
+		 copy instead, so as to keep LABEL pointing to the
+		 LABEL.  */
+	      gimple_stmt_iterator copy = label;
+	      gsi_move_after (&i, &copy);
+	      continue;
+	    }
+
+	  /* There aren't any (more?) debug stmts before label, so
+	     there isn't anything else to move after it.  */
+	  label = gsi_none ();
+	}
+    }
+
   make_blocks_1 (seq, ENTRY_BLOCK_PTR_FOR_FN (cfun));
 }
 
@@ -1005,11 +1066,7 @@ make_edges (void)
 	      tree target;
 
 	      if (!label_stmt)
-		{
-		  if (is_gimple_debug (gsi_stmt (gsi)))
-		    continue;
-		  break;
-		}
+		break;
 
 	      target = gimple_label_label (label_stmt);
 
@@ -1519,9 +1576,6 @@ cleanup_dead_labels (void)
 
       for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
 	{
-	  if (is_gimple_debug (gsi_stmt (i)))
-	    continue;
-
 	  tree label;
 	  glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (i));
 
@@ -1682,12 +1736,6 @@ cleanup_dead_labels (void)
 
       for (i = gsi_start_bb (bb); !gsi_end_p (i); )
 	{
-	  if (is_gimple_debug (gsi_stmt (i)))
-	    {
-	      gsi_next (&i);
-	      continue;
-	    }
-
 	  tree label;
 	  glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (i));
 
@@ -1863,8 +1911,6 @@ gimple_can_merge_blocks_p (basic_block a, basic_block b)
        gsi_next (&gsi))
     {
       tree lab;
-      if (is_gimple_debug (gsi_stmt (gsi)))
-	continue;
       glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (gsi));
       if (!label_stmt)
 	break;
@@ -5431,7 +5477,6 @@ verify_gimple_in_cfg (struct function *fn, bool verify_nothrow)
 	  err |= err2;
 	}
 
-      bool label_allowed = true;
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  gimple *stmt = gsi_stmt (gsi);
@@ -5448,19 +5493,6 @@ verify_gimple_in_cfg (struct function *fn, bool verify_nothrow)
 	      err2 = true;
 	    }
 
-	  /* Labels may be preceded only by debug markers, not debug bind
-	     or source bind or any other statements.  */
-	  if (gimple_code (stmt) == GIMPLE_LABEL)
-	    {
-	      if (!label_allowed)
-		{
-		  error ("gimple label in the middle of a basic block");
-		  err2 = true;
-		}
-	    }
-	  else if (!gimple_debug_begin_stmt_p (stmt))
-	    label_allowed = false;
-
 	  err2 |= verify_gimple_stmt (stmt);
 	  err2 |= verify_location (&blocks, gimple_location (stmt));
 
@@ -5584,10 +5616,6 @@ gimple_verify_flow_info (void)
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  tree label;
-
-	  if (is_gimple_debug (gsi_stmt (gsi)))
-	    continue;
-
 	  gimple *prev_stmt = stmt;
 
 	  stmt = gsi_stmt (gsi);
@@ -5912,10 +5940,8 @@ gimple_block_label (basic_block bb)
   tree label;
   glabel *stmt;
 
-  for (i = s; !gsi_end_p (i); gsi_next (&i))
+  for (i = s; !gsi_end_p (i); first = false, gsi_next (&i))
     {
-      if (is_gimple_debug (gsi_stmt (i)))
-	continue;
       stmt = dyn_cast <glabel *> (gsi_stmt (i));
       if (!stmt)
 	break;
@@ -5926,7 +5952,6 @@ gimple_block_label (basic_block bb)
 	    gsi_move_before (&i, &s);
 	  return label;
 	}
-      first = false;
     }
 
   label = create_artificial_label (UNKNOWN_LOCATION);
diff --git a/gcc/tree-cfgcleanup.c b/gcc/tree-cfgcleanup.c
index a0e5797ec0ec..bfcca03257d3 100644
--- a/gcc/tree-cfgcleanup.c
+++ b/gcc/tree-cfgcleanup.c
@@ -444,7 +444,7 @@ remove_forwarder_block (basic_block bb)
 {
   edge succ = single_succ_edge (bb), e, s;
   basic_block dest = succ->dest;
-  gimple *label;
+  gimple *stmt;
   edge_iterator ei;
   gimple_stmt_iterator gsi, gsi_to;
   bool can_move_debug_stmts;
@@ -457,9 +457,9 @@ remove_forwarder_block (basic_block bb)
 
   /* If the destination block consists of a nonlocal label or is a
      EH landing pad, do not merge it.  */
-  label = first_stmt (dest);
-  if (label)
-    if (glabel *label_stmt = dyn_cast <glabel *> (label))
+  stmt = first_stmt (dest);
+  if (stmt)
+    if (glabel *label_stmt = dyn_cast <glabel *> (stmt))
       if (DECL_NONLOCAL (gimple_label_label (label_stmt))
 	  || EH_LANDING_PAD_NR (gimple_label_label (label_stmt)) != 0)
 	return false;
@@ -536,28 +536,23 @@ remove_forwarder_block (basic_block bb)
      defined labels and labels with an EH landing pad number to the
      new block, so that the redirection of the abnormal edges works,
      jump targets end up in a sane place and debug information for
-     labels is retained.
-
-     While at that, move any debug stmts that appear before or in between
-     labels, but not those that can only appear after labels.  */
+     labels is retained.  */
   gsi_to = gsi_start_bb (dest);
-  gsi = gsi_start_bb (bb);
-  gimple_stmt_iterator gsie = gsi_after_labels (bb);
-  while (gsi_stmt (gsi) != gsi_stmt (gsie))
+  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
     {
-      tree decl;
-      label = gsi_stmt (gsi);
-      if (is_gimple_debug (label)
-	  ? can_move_debug_stmts
-	  : ((decl = gimple_label_label (as_a <glabel *> (label))),
-	     EH_LANDING_PAD_NR (decl) != 0
-	     || DECL_NONLOCAL (decl)
-	     || FORCED_LABEL (decl)
-	     || !DECL_ARTIFICIAL (decl)))
-	{
-	  gsi_remove (&gsi, false);
-	  gsi_insert_before (&gsi_to, label, GSI_SAME_STMT);
-	}
+      stmt = gsi_stmt (gsi);
+      if (is_gimple_debug (stmt))
+	break;
+
+      /* Forwarder blocks can only contain labels and debug stmts, and
+	 labels must come first, so if we get to this point, we know
+	 we're looking at a label.  */
+      tree decl = gimple_label_label (as_a <glabel *> (stmt));
+      if (EH_LANDING_PAD_NR (decl) != 0
+	  || DECL_NONLOCAL (decl)
+	  || FORCED_LABEL (decl)
+	  || !DECL_ARTIFICIAL (decl))
+	gsi_move_before (&gsi, &gsi_to);
       else
 	gsi_next (&gsi);
     }
@@ -565,14 +560,12 @@ remove_forwarder_block (basic_block bb)
   /* Move debug statements if the destination has a single predecessor.  */
   if (can_move_debug_stmts && !gsi_end_p (gsi))
     {
-      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
-      gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
+      gsi_to = gsi_after_labels (dest);
       do
 	{
 	  gimple *debug = gsi_stmt (gsi);
 	  gcc_assert (is_gimple_debug (debug));
-	  gsi_remove (&gsi, false);
-	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
+	  gsi_move_before (&gsi, &gsi_to);
 	}
       while (!gsi_end_p (gsi));
     }
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 743ee4e1cf99..fc94f5d83d2c 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -1295,14 +1295,14 @@ find_duplicate (same_succ *same_succ, basic_block bb1, basic_block bb2)
       tree label = gimple_label_label (as_a <glabel *> (gsi_stmt (gsi1)));
       if (DECL_NONLOCAL (label) || FORCED_LABEL (label))
 	return;
-      gsi_prev_nondebug (&gsi1);
+      gsi_prev (&gsi1);
     }
   while (!gsi_end_p (gsi2) && gimple_code (gsi_stmt (gsi2)) == GIMPLE_LABEL)
     {
       tree label = gimple_label_label (as_a <glabel *> (gsi_stmt (gsi2)));
       if (DECL_NONLOCAL (label) || FORCED_LABEL (label))
 	return;
-      gsi_prev_nondebug (&gsi2);
+      gsi_prev (&gsi2);
     }
   if (!(gsi_end_p (gsi1) && gsi_end_p (gsi2)))
     return;
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 4a96a039d14b..8bd840199d60 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -9479,24 +9479,6 @@ emit_notes_in_bb (basic_block bb, dataflow_set *set)
     }
 }
 
-/* Return BB's head, unless BB is the block that succeeds ENTRY_BLOCK,
-   in which case it searches back from BB's head for the very first
-   insn.  Use [get_first_insn (bb), BB_HEAD (bb->next_bb)[ as a range
-   to iterate over all insns of a function while iterating over its
-   BBs.  */
-
-static rtx_insn *
-get_first_insn (basic_block bb)
-{
-  rtx_insn *insn = BB_HEAD (bb);
-
-  if (bb->prev_bb == ENTRY_BLOCK_PTR_FOR_FN (cfun))
-    while (rtx_insn *prev = PREV_INSN (insn))
-      insn = prev;
-
-  return insn;
-}
-
 /* Emit notes for the whole function.  */
 
 static void
@@ -9525,8 +9507,7 @@ vt_emit_notes (void)
     {
       /* Emit the notes for changes of variable locations between two
 	 subsequent basic blocks.  */
-      emit_notes_for_differences (get_first_insn (bb),
-				  &cur, &VTI (bb)->in);
+      emit_notes_for_differences (BB_HEAD (bb), &cur, &VTI (bb)->in);
 
       if (MAY_HAVE_DEBUG_BIND_INSNS)
 	local_get_addr_cache = new hash_map<rtx, rtx>;
@@ -9929,7 +9910,7 @@ vt_init_cfa_base (void)
 /* Reemit INSN, a MARKER_DEBUG_INSN, as a note.  */
 
 static rtx_insn *
-reemit_marker_as_note (rtx_insn *insn, basic_block *bb)
+reemit_marker_as_note (rtx_insn *insn)
 {
   gcc_checking_assert (DEBUG_MARKER_INSN_P (insn));
 
@@ -9944,8 +9925,6 @@ reemit_marker_as_note (rtx_insn *insn, basic_block *bb)
 	  {
 	    note = emit_note_before (kind, insn);
 	    NOTE_MARKER_LOCATION (note) = INSN_LOCATION (insn);
-	    if (bb)
-	      BLOCK_FOR_INSN (note) = *bb;
 	  }
 	delete_insn (insn);
 	return note;
@@ -10153,39 +10132,11 @@ vt_initialize (void)
 	  HOST_WIDE_INT offset = VTI (bb)->out.stack_adjust;
 	  VTI (bb)->out.stack_adjust = VTI (bb)->in.stack_adjust;
 
-	  /* If we are walking the first basic block, walk any HEADER
-	     insns that might be before it too.  Unfortunately,
-	     BB_HEADER and BB_FOOTER are not set while we run this
-	     pass.  */
 	  rtx_insn *next;
-	  bool outside_bb = true;
-	  for (insn = get_first_insn (bb); insn != BB_HEAD (bb->next_bb);
-	       insn = next)
+	  FOR_BB_INSNS_SAFE (bb, insn, next)
 	    {
-	      if (insn == BB_HEAD (bb))
-		outside_bb = false;
-	      else if (insn == NEXT_INSN (BB_END (bb)))
-		outside_bb = true;
-	      next = NEXT_INSN (insn);
 	      if (INSN_P (insn))
 		{
-		  if (outside_bb)
-		    {
-		      /* Ignore non-debug insns outside of basic blocks.  */
-		      if (!DEBUG_INSN_P (insn))
-			continue;
-		      /* Debug binds shouldn't appear outside of bbs.  */
-		      gcc_assert (!DEBUG_BIND_INSN_P (insn));
-		    }
-		  basic_block save_bb = BLOCK_FOR_INSN (insn);
-		  if (!BLOCK_FOR_INSN (insn))
-		    {
-		      gcc_assert (outside_bb);
-		      BLOCK_FOR_INSN (insn) = bb;
-		    }
-		  else
-		    gcc_assert (BLOCK_FOR_INSN (insn) == bb);
-
 		  if (!frame_pointer_needed)
 		    {
 		      insn_stack_adjust_offset_pre_post (insn, &pre, &post);
@@ -10207,7 +10158,7 @@ vt_initialize (void)
 		  adjust_insn (bb, insn);
 		  if (DEBUG_MARKER_INSN_P (insn))
 		    {
-		      insn = reemit_marker_as_note (insn, &save_bb);
+		      reemit_marker_as_note (insn);
 		      continue;
 		    }
 
@@ -10259,7 +10210,6 @@ vt_initialize (void)
 			    }
 			}
 		    }
-		  BLOCK_FOR_INSN (insn) = save_bb;
 		}
 	    }
 	  gcc_assert (offset == VTI (bb)->out.stack_adjust);
@@ -10301,15 +10251,12 @@ delete_vta_debug_insns (void)
 
   FOR_EACH_BB_FN (bb, cfun)
     {
-      for (insn = get_first_insn (bb);
-	   insn != BB_HEAD (bb->next_bb)
-	     ? next = NEXT_INSN (insn), true : false;
-	   insn = next)
+      FOR_BB_INSNS_SAFE (bb, insn, next)
 	if (DEBUG_INSN_P (insn))
 	  {
 	    if (DEBUG_MARKER_INSN_P (insn))
 	      {
-		insn = reemit_marker_as_note (insn, NULL);
+		reemit_marker_as_note (insn);
 		continue;
 	      }
 
-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 20, 2017 at 04:57:43AM -0200, Alexandre Oliva wrote:
> for  gcc/ChangeLog
> 
> 	PR bootstrap/83396
> 	* cfgexpand.c (label_rtx_for_bb): Revert SFN changes that
> 	allowed debug stmts before labels.
> 	(expand_gimple_basic_block): Likewise.
> 	* gimple-iterator.c (gimple_find_edge_insert_loc): Likewise.
> 	* gimple-iterator.h (gsi_after_labels): Likewise.
> 	* tree-cfgcleanup (remove_forwarder_block): Likewise, but
> 	rename reused variable, and simplify using gsi_move_before.
> 	* tree-ssa-tail-merge.c (find_duplicate): Likewise.
> 	* tree-cfg.c (make_edges, cleanup_dead_labels): Likewise.
> 	(gimple_can_merge_blocks_p, verify_gimple_in_cfg): Likewise.
> 	(gimple_verify_flow_info, gimple_block_label): Likewise.
> 	(make_blocks): Move debug markers after adjacent labels.
> 	* cfgrtl.c (skip_insns_after_block): Revert SFN changes that
> 	allowed debug insns outside blocks.
> 	* df-scan.c (df_insn_delete): Likewise.
> 	* lra-constraints.c (update_ebb_live_info): Likewise.
> 	* var-tracking.c (get_first_insn, vt_emit_notes): Likewise.
> 	(vt_initialize, delete_vta_debug_insns): Likewise.
> 	(reemit_marker_as_note): Drop BB parm.  Adjust callers.

Ok, thanks.

	Jakub

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 12, 2017, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:

> Hi David,
>> Something in this series broke bootstrap on AIX, probably Power in general.

> I'm seeing the same in a sparc-sun-solaris2.11 bootstrap.

The AIX patch, that I've just emailed out in this thread, should fix
that as well.  As for the regression you reported, here's a fix.
Regstrapping; ok to install?


[SFN] don't assume BLOCK_FOR_INSN is set in var-tracking

There's no guarantee that BLOCK_FOR_INSN will be set before var-tracking.
So, keep track of whether we're in the first block header or inside a BB
explicitly, and apply the logic we meant to apply outside BBs only when
we are indeed outside a BB.

for  gcc/ChangeLog

	PR bootstrap/83396
	* var-tracking.c (vt_initialize): Keep track of BB boundaries.
---
 gcc/var-tracking.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 8e500b144712..12158dbb1e0d 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -10156,12 +10156,16 @@ vt_initialize (void)
 	  /* If we are walking the first basic block, walk any HEADER
 	     insns that might be before it too.  Unfortunately,
 	     BB_HEADER and BB_FOOTER are not set while we run this
-	     pass.  */
+	     pass.  Unfortunately, BLOCK_FOR_INSN may not be set, so
+	     we can't assume that its being NULL implies we're outside
+	     the basic block.  */
 	  insn = get_first_insn (bb);
+	  bool outside_bb = insn != BB_HEAD (bb);
 	  for (rtx_insn *next;
 	       insn != BB_HEAD (bb->next_bb)
 		 ? next = NEXT_INSN (insn), true : false;
-	       insn = next)
+	       insn = next,
+		 outside_bb = outside_bb && next != BB_HEAD (bb))
 	    {
 	      if (INSN_P (insn))
 		{
@@ -10169,11 +10173,11 @@ vt_initialize (void)
 		  if (!BLOCK_FOR_INSN (insn))
 		    {
 		      BLOCK_FOR_INSN (insn) = bb;
-		      gcc_assert (DEBUG_INSN_P (insn));
+		      gcc_assert (!outside_bb || DEBUG_INSN_P (insn));
 		      /* Reset debug insns between basic blocks.
 			 Their location is not reliable, because they
 			 were probably not maintained up to date.  */
-		      if (DEBUG_BIND_INSN_P (insn))
+		      if (outside_bb && DEBUG_BIND_INSN_P (insn))
 			INSN_VAR_LOCATION_LOC (insn)
 			  = gen_rtx_UNKNOWN_VAR_LOC ();
 		    }


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Rainer Orth]

Hi Alexandre Oliva <aoliva@redhat.com> writes:

> On Dec 12, 2017, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
>
>> Hi David,
>>> Something in this series broke bootstrap on AIX, probably Power in general.
>
>> I'm seeing the same in a sparc-sun-solaris2.11 bootstrap.
>
> The AIX patch, that I've just emailed out in this thread, should fix
> that as well.  As for the regression you reported, here's a fix.

it did indeed, and this one fixed the testsuite regression, as just
confirmed in a sparc-sun-solaris2.11 bootstrap.

Thanks.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 05:22:32AM -0200, Alexandre Oliva wrote:
> On Dec 12, 2017, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
> 
> > Hi David,
> >> Something in this series broke bootstrap on AIX, probably Power in general.
> 
> > I'm seeing the same in a sparc-sun-solaris2.11 bootstrap.
> 
> The AIX patch, that I've just emailed out in this thread, should fix
> that as well.  As for the regression you reported, here's a fix.
> Regstrapping; ok to install?
> 
> 
> [SFN] don't assume BLOCK_FOR_INSN is set in var-tracking
> 
> There's no guarantee that BLOCK_FOR_INSN will be set before var-tracking.
> So, keep track of whether we're in the first block header or inside a BB
> explicitly, and apply the logic we meant to apply outside BBs only when
> we are indeed outside a BB.
> 
> for  gcc/ChangeLog
> 
> 	PR bootstrap/83396
> 	* var-tracking.c (vt_initialize): Keep track of BB boundaries.

This looks like a workaround for a bigger problem.

In particular, this testcase is using selective scheduling, therefore
we turn off -fvar-tracking-assignments, but the debug stmt markers are
emitted anyway.

-fvar-tracking is still true, so the var-tracking pass does everything it
normally does (successfully), then the free_cfg pass removes all
BLOCK_FOR_INSN notes, then some targets in their machine reorg recompute
those, but sparc apparently doesn't, and finally in final.c:

  /* Turn debug markers into notes.  */
  if (!MAY_HAVE_DEBUG_BIND_INSNS && MAY_HAVE_DEBUG_MARKER_INSNS)
    variable_tracking_main ();

Eeek, this runs all of the var tracking again, even when it has been done
earlier, but this time without BLOCK_FOR_INSN.

This is just wrong.  So, I think the right fix here is instead (so far
tested just with sparc cross-compiler on the single testcase).

Or export delete_vta_debug_insns function and call that under that condition
instead of variable_tracking_main, which will do the same thing for
!flag_var_tracking.

2017-12-13  Jakub Jelinek  <jakub@redhat.com>

	PR bootstrap/83396
	* final.c (rest_of_handle_final): Call variable_tracking_main only
	if !flag_var_tracking.

--- gcc/final.c.jj	2017-12-12 09:48:15.000000000 +0100
+++ gcc/final.c	2017-12-13 11:29:12.284676265 +0100
@@ -4541,8 +4541,9 @@ rest_of_handle_final (void)
 {
   const char *fnname = get_fnname_from_decl (current_function_decl);
 
-  /* Turn debug markers into notes.  */
-  if (!MAY_HAVE_DEBUG_BIND_INSNS && MAY_HAVE_DEBUG_MARKER_INSNS)
+  /* Turn debug markers into notes if the var-tracking pass has not
+     been invoked.  */
+  if (!flag_var_tracking && MAY_HAVE_DEBUG_MARKER_INSNS)
     variable_tracking_main ();
 
   assemble_start_function (current_function_decl, fnname);


	Jakub

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 11:34:04AM +0100, Jakub Jelinek wrote:
> 2017-12-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR bootstrap/83396
> 	* final.c (rest_of_handle_final): Call variable_tracking_main only
> 	if !flag_var_tracking.

Bootstrapped/regtested on x86_64-linux, i686-linux and powerpc64le-linux,
powerpc64-linux regtest still pending, ok for trunk?

> --- gcc/final.c.jj	2017-12-12 09:48:15.000000000 +0100
> +++ gcc/final.c	2017-12-13 11:29:12.284676265 +0100
> @@ -4541,8 +4541,9 @@ rest_of_handle_final (void)
>  {
>    const char *fnname = get_fnname_from_decl (current_function_decl);
>  
> -  /* Turn debug markers into notes.  */
> -  if (!MAY_HAVE_DEBUG_BIND_INSNS && MAY_HAVE_DEBUG_MARKER_INSNS)
> +  /* Turn debug markers into notes if the var-tracking pass has not
> +     been invoked.  */
> +  if (!flag_var_tracking && MAY_HAVE_DEBUG_MARKER_INSNS)
>      variable_tracking_main ();
>  
>    assemble_start_function (current_function_decl, fnname);
> 

	Jakub

Re: [SFN] Bootstrap broken

[Richard Biener]

On Wed, 13 Dec 2017, Jakub Jelinek wrote:

> On Wed, Dec 13, 2017 at 11:34:04AM +0100, Jakub Jelinek wrote:
> > 2017-12-13  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR bootstrap/83396
> > 	* final.c (rest_of_handle_final): Call variable_tracking_main only
> > 	if !flag_var_tracking.
> 
> Bootstrapped/regtested on x86_64-linux, i686-linux and powerpc64le-linux,
> powerpc64-linux regtest still pending, ok for trunk?

Ok.

Richard.

> > --- gcc/final.c.jj	2017-12-12 09:48:15.000000000 +0100
> > +++ gcc/final.c	2017-12-13 11:29:12.284676265 +0100
> > @@ -4541,8 +4541,9 @@ rest_of_handle_final (void)
> >  {
> >    const char *fnname = get_fnname_from_decl (current_function_decl);
> >  
> > -  /* Turn debug markers into notes.  */
> > -  if (!MAY_HAVE_DEBUG_BIND_INSNS && MAY_HAVE_DEBUG_MARKER_INSNS)
> > +  /* Turn debug markers into notes if the var-tracking pass has not
> > +     been invoked.  */
> > +  if (!flag_var_tracking && MAY_HAVE_DEBUG_MARKER_INSNS)
> >      variable_tracking_main ();
> >  
> >    assemble_start_function (current_function_decl, fnname);
> > 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 13, 2017, Jakub Jelinek <jakub@redhat.com> wrote:

> In particular, this testcase is using selective scheduling, therefore
> we turn off -fvar-tracking-assignments, but the debug stmt markers are
> emitted anyway.

*nod*, that much was intended (though I could be convinced to change it ;-)

> -fvar-tracking is still true, so the var-tracking pass does everything it
> normally does (successfully), then the free_cfg pass removes all
> BLOCK_FOR_INSN notes, then some targets in their machine reorg recompute
> those, but sparc apparently doesn't, and finally in final.c:

>   /* Turn debug markers into notes.  */
>   if (!MAY_HAVE_DEBUG_BIND_INSNS && MAY_HAVE_DEBUG_MARKER_INSNS)
>     variable_tracking_main ();

> Eeek, this runs all of the var tracking again,

Eeek, indeed ;-)  /me takes a mental note that flag_var_tracking !=
> MAY_HAVE_DEBUG_BIND_INSNS, and underlines it several times ;-D

Thanks for spotting the deeper problem and for fixing it!

I'm glad the patch I posted to fix the shallower one still serves as a
basis for the ongoing attempts to fix one of the remaining ia64 issues.
I'm on it.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

On Dec 12, 2017, David Edelsohn <dje.gcc@gmail.com> wrote:

> Something in this series broke bootstrap on AIX, probably Power in general.

And probably a number of other platforms as well.

Here's a patch that seems to have fixed lots of problems.  Without it,
we might move debug (bind) stmts from a forwarder block about to be
removed, inserting them before the label of a successor block.  debug
bind stmts before labels are not welcome (they might become insns
outside any blocks, and not be properly adjusted; in this specific
testcase, a reg lowered to a concatn remained shared because the
out-of-block debug insn was not adjusted), and I'm reconsidering even
debug markers, but for now, I'm leaving markers before and among labels.

To fix the problem, I've rearranged the forwarder block remover to
insert stmts that were before or among labels before labels, but those
that are after labels (or in a block without labels) are inserted after
any labels in the destination block.  This should keep debug insns in
order, except when the forwarder block has no labels, whereas the
successor block had markers before the labels.  I'm undecided between
moving all markers in the successor after any labels in it, before
making other changes, or to rule out markers before or among labels
altogether.

For now, to restore bootstrap and builds on most platforms, this will
do.  There are still unrelated -fcompare-debug issues in the supplied
AIX testcase, but Jakub's reduced testcase passes even -fcompare-debug.

Regstrapping on x86_64-linux-gnu and i686-linux-gnu.  Hopefully I'll
have feedback about its bootstrap on powerpc-aix and sparc-solaris as
well before it goes in.  Ok to install?


[SFN] don't move after-label debug stmts before labels

When removing a forwarder block (that contains debug stmts), be careful
to not insert before labels debug stmts that appear after labels.

for  gcc/ChangeLog

	PR bootstrap/83396
	PR debug/83391
	* tree-cfgcleanup.c (remove_forwarder_block): Keep after
	labels debug stmts appearing after labels.

for  gcc/testsuite/ChangeLog

	PR bootstrap/83396
	PR debug/83391
	* gcc.dg/torture/pr83396.c: New.
	* g++.dg/torture/pr83391.C: New.
---
 gcc/testsuite/g++.dg/torture/pr83391.C |   34 +++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/torture/pr83396.c |   37 ++++++++++++++++++++++++++++++++
 gcc/tree-cfgcleanup.c                  |   29 ++++++++++++++++++++++---
 3 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/torture/pr83391.C
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr83396.c

diff --git a/gcc/testsuite/g++.dg/torture/pr83391.C b/gcc/testsuite/g++.dg/torture/pr83391.C
new file mode 100644
index 000000000000..098a1101a3f4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr83391.C
@@ -0,0 +1,34 @@
+/* PR debug/83391 */
+/* { dg-do compile } */
+
+unsigned char a;
+enum E { F, G, H } b;
+int c, d;
+
+void
+foo ()
+{
+  int e;
+  bool f;
+  E g = b;
+  while (1)
+    {
+      unsigned char h = a ? d : 0;
+      switch (g)
+	{
+	case 0:
+	  f = h <= 'Z' || h >= 'a' && h <= 'z';
+	  break;
+	case 1:
+	  {
+	    unsigned char i = h;
+	    e = 0;
+	  }
+	  if (e || h)
+	    g = H;
+	  /* FALLTHRU */
+	default:
+	  c = 0;
+	}
+    }
+}
diff --git a/gcc/testsuite/gcc.dg/torture/pr83396.c b/gcc/testsuite/gcc.dg/torture/pr83396.c
new file mode 100644
index 000000000000..4c0c30ceaab3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr83396.c
@@ -0,0 +1,37 @@
+/* PR bootstrap/83396 */
+/* { dg-do compile } */
+
+int fn1 (void);
+void fn2 (void *, const char *);
+void fn3 (void);
+
+void
+fn4 (long long x)
+{
+  fn3 ();
+}
+
+void
+fn5 (long long x)
+{
+  if (x)
+    fn3();
+}
+
+void
+fn6 (long long x)
+{
+  switch (fn1 ())
+    {
+    case 0:
+      fn5 (x);
+    case 2:
+      fn2 (0, "");
+      break;
+    case 1:
+    case 3:
+      fn4(x);
+    case 5:
+      fn2 (0, "");
+    }
+}
diff --git a/gcc/tree-cfgcleanup.c b/gcc/tree-cfgcleanup.c
index 0bee21756f2b..e30a93d504b6 100644
--- a/gcc/tree-cfgcleanup.c
+++ b/gcc/tree-cfgcleanup.c
@@ -536,14 +536,23 @@ remove_forwarder_block (basic_block bb)
      defined labels and labels with an EH landing pad number to the
      new block, so that the redirection of the abnormal edges works,
      jump targets end up in a sane place and debug information for
-     labels is retained.  */
+     labels is retained.
+
+     While at that, move any debug stmts that appear before or among
+     labels, but not those that can only appear after labels, unless
+     the destination block didn't have labels of its own.  */
   gsi_to = gsi_start_bb (dest);
-  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
+  gsi = gsi_start_bb (bb);
+  gimple_stmt_iterator gsie = gsi_after_labels (bb);
+  gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
+  bool can_move_early_debug_stmts = can_move_debug_stmts
+    && (gsi_stmt (gsi) != gsi_stmt (gsie) || gsi_stmt (gsi_to) != gsi_stmt (gsie_to));
+  while (gsi_stmt (gsi) != gsi_stmt (gsie))
     {
       tree decl;
       label = gsi_stmt (gsi);
       if (is_gimple_debug (label)
-	  ? can_move_debug_stmts
+	  ? can_move_early_debug_stmts
 	  : ((decl = gimple_label_label (as_a <glabel *> (label))),
 	     EH_LANDING_PAD_NR (decl) != 0
 	     || DECL_NONLOCAL (decl)
@@ -557,6 +566,20 @@ remove_forwarder_block (basic_block bb)
 	gsi_next (&gsi);
     }
 
+  /* Move debug statements if the destination has a single predecessor.  */
+  if (can_move_debug_stmts && !gsi_end_p (gsi))
+    {
+      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
+      do
+	{
+	  gimple *debug = gsi_stmt (gsi);
+	  gcc_assert (is_gimple_debug (debug));
+	  gsi_remove (&gsi, false);
+	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
+	}
+      while (!gsi_end_p (gsi));
+    }
+
   bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
 
   /* Update the dominators.  */


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 05:21:01AM -0200, Alexandre Oliva wrote:
> index 000000000000..098a1101a3f4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr83391.C
> @@ -0,0 +1,34 @@
> +/* PR debug/83391 */
> +/* { dg-do compile } */

If you put this into dg-torture.exp, please add:
/* { dg-options "-g" } */
and readd the needed:
/* { dg-additional-options "-mbranch-cost=1" { target { i?86-*-* x86_64-*-* mips*-*-* s390*-*-* avr*-*-* } } } */

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr83396.c
> @@ -0,0 +1,37 @@
> +/* PR bootstrap/83396 */
> +/* { dg-do compile } */

Please add -g.

> --- a/gcc/tree-cfgcleanup.c
> +++ b/gcc/tree-cfgcleanup.c
> @@ -536,14 +536,23 @@ remove_forwarder_block (basic_block bb)
>       defined labels and labels with an EH landing pad number to the
>       new block, so that the redirection of the abnormal edges works,
>       jump targets end up in a sane place and debug information for
> -     labels is retained.  */
> +     labels is retained.
> +
> +     While at that, move any debug stmts that appear before or among
> +     labels, but not those that can only appear after labels, unless
> +     the destination block didn't have labels of its own.  */
>    gsi_to = gsi_start_bb (dest);
> -  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> +  gsi = gsi_start_bb (bb);
> +  gimple_stmt_iterator gsie = gsi_after_labels (bb);
> +  gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
> +  bool can_move_early_debug_stmts = can_move_debug_stmts
> +    && (gsi_stmt (gsi) != gsi_stmt (gsie) || gsi_stmt (gsi_to) != gsi_stmt (gsie_to));

Formatting, this should be
  bool can_move_early_debug_stmts
    = ...
and the line is too long, so needs to be wrapped.

Furthermore, I must say I don't understand why
can_move_early_debug_stmts should care whether there are any labels in
dest bb or not.  That sounds very risky for introducing non-# DEBUG BEGIN_STMT
debug insns before labels if it could happen.  Though, if
gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
do anything and nothing cares about can_move_early_debug_stmts afterwards.
So, in short, can_move_early_debug_stmts is used only if
gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);

So, can we get rid of can_move_early_debug_stmts altogether and just use
can_move_debug_stmts in there instead?

Another thing I find risky is that you compute gsie_to so early and don't
update it.  If you don't need it above for can_move_early_debug_stmts, can
you just do it back where it used to be done,

> +  while (gsi_stmt (gsi) != gsi_stmt (gsie))
>      {
>        tree decl;
>        label = gsi_stmt (gsi);
>        if (is_gimple_debug (label)
> -	  ? can_move_debug_stmts
> +	  ? can_move_early_debug_stmts
>  	  : ((decl = gimple_label_label (as_a <glabel *> (label))),
>  	     EH_LANDING_PAD_NR (decl) != 0
>  	     || DECL_NONLOCAL (decl)
> @@ -557,6 +566,20 @@ remove_forwarder_block (basic_block bb)
>  	gsi_next (&gsi);
>      }
>  
> +  /* Move debug statements if the destination has a single predecessor.  */
> +  if (can_move_debug_stmts && !gsi_end_p (gsi))
> +    {
> +      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));

i.e. here?

> +      do
> +	{
> +	  gimple *debug = gsi_stmt (gsi);
> +	  gcc_assert (is_gimple_debug (debug));
> +	  gsi_remove (&gsi, false);
> +	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
> +	}
> +      while (!gsi_end_p (gsi));
> +    }
> +
>    bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
>  
>    /* Update the dominators.  */

	Jakub

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 10:28:22AM +0100, Jakub Jelinek wrote:
> Formatting, this should be
>   bool can_move_early_debug_stmts
>     = ...
> and the line is too long, so needs to be wrapped.
> 
> Furthermore, I must say I don't understand why
> can_move_early_debug_stmts should care whether there are any labels in
> dest bb or not.  That sounds very risky for introducing non-# DEBUG BEGIN_STMT
> debug insns before labels if it could happen.  Though, if
> gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
> do anything and nothing cares about can_move_early_debug_stmts afterwards.
> So, in short, can_move_early_debug_stmts is used only if
> gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
> can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);
> 
> So, can we get rid of can_move_early_debug_stmts altogether and just use
> can_move_debug_stmts in there instead?
> 
> Another thing I find risky is that you compute gsie_to so early and don't
> update it.  If you don't need it above for can_move_early_debug_stmts, can
> you just do it back where it used to be done,

Here it is everything in patch form, in case some volunteers are willing to
test it on their targets, because we need faster turn-arounds for this.

2017-12-13  Alexandre Oliva <aoliva@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR bootstrap/83396
	PR debug/83391
	* tree-cfgcleanup.c (remove_forwarder_block): Keep after
	labels debug stmts that can only appear after labels.

        * gcc.dg/torture/pr83396.c: New test.
        * g++.dg/torture/pr83391.C: New test.

--- gcc/tree-cfgcleanup.c.jj	2017-12-12 09:48:26.813393301 +0100
+++ gcc/tree-cfgcleanup.c	2017-12-13 11:39:03.373065381 +0100
@@ -536,9 +536,14 @@ remove_forwarder_block (basic_block bb)
      defined labels and labels with an EH landing pad number to the
      new block, so that the redirection of the abnormal edges works,
      jump targets end up in a sane place and debug information for
-     labels is retained.  */
+     labels is retained.
+
+     While at that, move any debug stmts that appear before or in between
+     labels, but not those that can only appear after labels.  */
   gsi_to = gsi_start_bb (dest);
-  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
+  gsi = gsi_start_bb (bb);
+  gimple_stmt_iterator gsie = gsi_after_labels (bb);
+  while (gsi_stmt (gsi) != gsi_stmt (gsie))
     {
       tree decl;
       label = gsi_stmt (gsi);
@@ -557,6 +562,21 @@ remove_forwarder_block (basic_block bb)
 	gsi_next (&gsi);
     }
 
+  /* Move debug statements if the destination has a single predecessor.  */
+  if (can_move_debug_stmts && !gsi_end_p (gsi))
+    {
+      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
+      gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
+      do
+	{
+	  gimple *debug = gsi_stmt (gsi);
+	  gcc_assert (is_gimple_debug (debug));
+	  gsi_remove (&gsi, false);
+	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
+	}
+      while (!gsi_end_p (gsi));
+    }
+
   bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
 
   /* Update the dominators.  */
--- gcc/testsuite/g++.dg/torture/pr83391.C.jj
+++ gcc/testsuite/g++.dg/torture/pr83391.C
@@ -0,0 +1,36 @@
+// PR debug/83391
+// { dg-do compile }
+// { dg-options "-g" }
+// { dg-additional-options "-mbranch-cost=1" { target { i?86-*-* x86_64-*-* mips*-*-* s390*-*-* avr*-*-* } } }
+
+unsigned char a;
+enum E { F, G, H } b;
+int c, d;
+
+void
+foo ()
+{
+  int e;
+  bool f;
+  E g = b;
+  while (1)
+    {
+      unsigned char h = a ? d : 0;
+      switch (g)
+	{
+	case 0:
+	  f = h <= 'Z' || h >= 'a' && h <= 'z';
+	  break;
+	case 1:
+	  {
+	    unsigned char i = h;
+	    e = 0;
+	  }
+	  if (e || h)
+	    g = H;
+	  /* FALLTHRU */
+	default:
+	  c = 0;
+	}
+    }
+}
--- gcc/testsuite/gcc.dg/torture/pr83396.c.jj
+++ gcc/testsuite/gcc.dg/torture/pr83396.c
@@ -0,0 +1,38 @@
+/* PR bootstrap/83396 */
+/* { dg-do compile } */
+/* { dg-options "-g" } */
+
+int fn1 (void);
+void fn2 (void *, const char *);
+void fn3 (void);
+
+void
+fn4 (long long x)
+{
+  fn3 ();
+}
+
+void
+fn5 (long long x)
+{
+  if (x)
+    fn3();
+}
+
+void
+fn6 (long long x)
+{
+  switch (fn1 ())
+    {
+    case 0:
+      fn5 (x);
+    case 2:
+      fn2 (0, "");
+      break;
+    case 1:
+    case 3:
+      fn4(x);
+    case 5:
+      fn2 (0, "");
+    }
+}


	Jakub

Re: [SFN] Bootstrap broken

[Rainer Orth]

Hi Jakub,

> Here it is everything in patch form, in case some volunteers are willing to
> test it on their targets, because we need faster turn-arounds for this.

thanks for that: it's easy to loose track in this maze ;-)

I've just bootstrapped sparc-sun-solaris2.11 with your patch and this one:

	https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00794.html

The bootstrap succeeds, but the gcc.c-torture/compile/pr69102.c
regression persists.  Besides, I see

+FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times graphite "2 loops carried no dependency" 1 (found 0 times)
+FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times optimized "loopfn.1" 4 (found 0 times)
+FAIL: libgomp.graphite/force-parallel-8.c scan-tree-dump-times graphite "5 loops carried no dependency" 1 (found 0 times)

which is most likely unrelated (I upgraded the tree from r255584 to
r255603).

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 02:31:07PM +0100, Rainer Orth wrote:
> Hi Jakub,
> 
> > Here it is everything in patch form, in case some volunteers are willing to
> > test it on their targets, because we need faster turn-arounds for this.
> 
> thanks for that: it's easy to loose track in this maze ;-)

True.  What I'm regtesting (bootstraps already done) on
{x86_64,i686,powerpc64{,le}}-linux now is:
https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00811.html
https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00808.html
https://gcc.gnu.org/bugzilla/attachment.cgi?id=42861
https://gcc.gnu.org/bugzilla/attachment.cgi?id=42866
https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00794.html
set.  Does pr69102.c FAIL with that set?

> I've just bootstrapped sparc-sun-solaris2.11 with your patch and this one:
> 
> 	https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00794.html
> 
> The bootstrap succeeds, but the gcc.c-torture/compile/pr69102.c
> regression persists.  Besides, I see
> 
> +FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times graphite "2 loops carried no dependency" 1 (found 0 times)
> +FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times optimized "loopfn.1" 4 (found 0 times)
> +FAIL: libgomp.graphite/force-parallel-8.c scan-tree-dump-times graphite "5 loops carried no dependency" 1 (found 0 times)
> 
> which is most likely unrelated (I upgraded the tree from r255584 to
> r255603).

Yeah, these are almost certainly unrelated.

	Jakub

Re: [SFN] Bootstrap broken

[Rainer Orth]

Hi Jakub,

> On Wed, Dec 13, 2017 at 02:31:07PM +0100, Rainer Orth wrote:
>> Hi Jakub,
>> 
>> > Here it is everything in patch form, in case some volunteers are willing to
>> > test it on their targets, because we need faster turn-arounds for this.
>> 
>> thanks for that: it's easy to loose track in this maze ;-)
>
> True.  What I'm regtesting (bootstraps already done) on
> {x86_64,i686,powerpc64{,le}}-linux now is:
> https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00811.html
> https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00808.html
> https://gcc.gnu.org/bugzilla/attachment.cgi?id=42861
> https://gcc.gnu.org/bugzilla/attachment.cgi?id=42866
> https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00794.html
> set.  Does pr69102.c FAIL with that set?

thanks for the list.  A sparc-sun-solaris2.11 bootstrap with the whole
set is now running; expect results in about two hours.

>> I've just bootstrapped sparc-sun-solaris2.11 with your patch and this one:
>> 
>> 	https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00794.html
>> 
>> The bootstrap succeeds, but the gcc.c-torture/compile/pr69102.c
>> regression persists.  Besides, I see
>> 
>> +FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times graphite
>> "2 loops carried no dependency" 1 (found 0 times)
>> +FAIL: libgomp.graphite/force-parallel-4.c scan-tree-dump-times optimized
>> "loopfn.1" 4 (found 0 times)
>> +FAIL: libgomp.graphite/force-parallel-8.c scan-tree-dump-times graphite
>> "5 loops carried no dependency" 1 (found 0 times)
>> 
>> which is most likely unrelated (I upgraded the tree from r255584 to
>> r255603).
>
> Yeah, these are almost certainly unrelated.

It certainly is: I've filed PR tree-optimization/83410.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [SFN] Bootstrap broken

[Rainer Orth]

Hi Jakub,

>> On Wed, Dec 13, 2017 at 02:31:07PM +0100, Rainer Orth wrote:
>>> Hi Jakub,
>>> 
>>> > Here it is everything in patch form, in case some volunteers are willing to
>>> > test it on their targets, because we need faster turn-arounds for this.
>>> 
>>> thanks for that: it's easy to loose track in this maze ;-)
>>
>> True.  What I'm regtesting (bootstraps already done) on
>> {x86_64,i686,powerpc64{,le}}-linux now is:
>> https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00811.html
>> https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00808.html
>> https://gcc.gnu.org/bugzilla/attachment.cgi?id=42861
>> https://gcc.gnu.org/bugzilla/attachment.cgi?id=42866
>> https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00794.html
>> set.  Does pr69102.c FAIL with that set?
>
> thanks for the list.  A sparc-sun-solaris2.11 bootstrap with the whole
> set is now running; expect results in about two hours.

completed now and the testsuite regression is gone, too.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [SFN] Bootstrap broken

[Jakub Jelinek]

On Wed, Dec 13, 2017 at 11:45:51AM +0100, Jakub Jelinek wrote:
> On Wed, Dec 13, 2017 at 10:28:22AM +0100, Jakub Jelinek wrote:
> > Formatting, this should be
> >   bool can_move_early_debug_stmts
> >     = ...
> > and the line is too long, so needs to be wrapped.
> > 
> > Furthermore, I must say I don't understand why
> > can_move_early_debug_stmts should care whether there are any labels in
> > dest bb or not.  That sounds very risky for introducing non-# DEBUG BEGIN_STMT
> > debug insns before labels if it could happen.  Though, if
> > gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
> > do anything and nothing cares about can_move_early_debug_stmts afterwards.
> > So, in short, can_move_early_debug_stmts is used only if
> > gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
> > can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);
> > 
> > So, can we get rid of can_move_early_debug_stmts altogether and just use
> > can_move_debug_stmts in there instead?
> > 
> > Another thing I find risky is that you compute gsie_to so early and don't
> > update it.  If you don't need it above for can_move_early_debug_stmts, can
> > you just do it back where it used to be done,
> 
> Here it is everything in patch form, in case some volunteers are willing to
> test it on their targets, because we need faster turn-arounds for this.
> 
> 2017-12-13  Alexandre Oliva <aoliva@redhat.com>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR bootstrap/83396
> 	PR debug/83391
> 	* tree-cfgcleanup.c (remove_forwarder_block): Keep after
> 	labels debug stmts that can only appear after labels.
> 
> 	* gcc.dg/torture/pr83396.c: New test.
> 	* g++.dg/torture/pr83391.C: New test.

Bootstrapped/regtested on x86_64-linux, i686-linux and powerpc64le-linux,
powerpc64-linux regtest still pending, ok for trunk?

> --- gcc/tree-cfgcleanup.c.jj	2017-12-12 09:48:26.813393301 +0100
> +++ gcc/tree-cfgcleanup.c	2017-12-13 11:39:03.373065381 +0100
> @@ -536,9 +536,14 @@ remove_forwarder_block (basic_block bb)
>       defined labels and labels with an EH landing pad number to the
>       new block, so that the redirection of the abnormal edges works,
>       jump targets end up in a sane place and debug information for
> -     labels is retained.  */
> +     labels is retained.
> +
> +     While at that, move any debug stmts that appear before or in between
> +     labels, but not those that can only appear after labels.  */
>    gsi_to = gsi_start_bb (dest);
> -  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> +  gsi = gsi_start_bb (bb);
> +  gimple_stmt_iterator gsie = gsi_after_labels (bb);
> +  while (gsi_stmt (gsi) != gsi_stmt (gsie))
>      {
>        tree decl;
>        label = gsi_stmt (gsi);
> @@ -557,6 +562,21 @@ remove_forwarder_block (basic_block bb)
>  	gsi_next (&gsi);
>      }
>  
> +  /* Move debug statements if the destination has a single predecessor.  */
> +  if (can_move_debug_stmts && !gsi_end_p (gsi))
> +    {
> +      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
> +      gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
> +      do
> +	{
> +	  gimple *debug = gsi_stmt (gsi);
> +	  gcc_assert (is_gimple_debug (debug));
> +	  gsi_remove (&gsi, false);
> +	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
> +	}
> +      while (!gsi_end_p (gsi));
> +    }
> +
>    bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
>  
>    /* Update the dominators.  */
> --- gcc/testsuite/g++.dg/torture/pr83391.C.jj
> +++ gcc/testsuite/g++.dg/torture/pr83391.C
> @@ -0,0 +1,36 @@
> +// PR debug/83391
> +// { dg-do compile }
> +// { dg-options "-g" }
> +// { dg-additional-options "-mbranch-cost=1" { target { i?86-*-* x86_64-*-* mips*-*-* s390*-*-* avr*-*-* } } }
> +
> +unsigned char a;
> +enum E { F, G, H } b;
> +int c, d;
> +
> +void
> +foo ()
> +{
> +  int e;
> +  bool f;
> +  E g = b;
> +  while (1)
> +    {
> +      unsigned char h = a ? d : 0;
> +      switch (g)
> +	{
> +	case 0:
> +	  f = h <= 'Z' || h >= 'a' && h <= 'z';
> +	  break;
> +	case 1:
> +	  {
> +	    unsigned char i = h;
> +	    e = 0;
> +	  }
> +	  if (e || h)
> +	    g = H;
> +	  /* FALLTHRU */
> +	default:
> +	  c = 0;
> +	}
> +    }
> +}
> --- gcc/testsuite/gcc.dg/torture/pr83396.c.jj
> +++ gcc/testsuite/gcc.dg/torture/pr83396.c
> @@ -0,0 +1,38 @@
> +/* PR bootstrap/83396 */
> +/* { dg-do compile } */
> +/* { dg-options "-g" } */
> +
> +int fn1 (void);
> +void fn2 (void *, const char *);
> +void fn3 (void);
> +
> +void
> +fn4 (long long x)
> +{
> +  fn3 ();
> +}
> +
> +void
> +fn5 (long long x)
> +{
> +  if (x)
> +    fn3();
> +}
> +
> +void
> +fn6 (long long x)
> +{
> +  switch (fn1 ())
> +    {
> +    case 0:
> +      fn5 (x);
> +    case 2:
> +      fn2 (0, "");
> +      break;
> +    case 1:
> +    case 3:
> +      fn4(x);
> +    case 5:
> +      fn2 (0, "");
> +    }
> +}
> 
> 
> 	Jakub

	Jakub

Re: [SFN] Bootstrap broken

[Richard Biener]

On Wed, 13 Dec 2017, Jakub Jelinek wrote:

> On Wed, Dec 13, 2017 at 11:45:51AM +0100, Jakub Jelinek wrote:
> > On Wed, Dec 13, 2017 at 10:28:22AM +0100, Jakub Jelinek wrote:
> > > Formatting, this should be
> > >   bool can_move_early_debug_stmts
> > >     = ...
> > > and the line is too long, so needs to be wrapped.
> > > 
> > > Furthermore, I must say I don't understand why
> > > can_move_early_debug_stmts should care whether there are any labels in
> > > dest bb or not.  That sounds very risky for introducing non-# DEBUG BEGIN_STMT
> > > debug insns before labels if it could happen.  Though, if
> > > gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
> > > do anything and nothing cares about can_move_early_debug_stmts afterwards.
> > > So, in short, can_move_early_debug_stmts is used only if
> > > gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
> > > can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);
> > > 
> > > So, can we get rid of can_move_early_debug_stmts altogether and just use
> > > can_move_debug_stmts in there instead?
> > > 
> > > Another thing I find risky is that you compute gsie_to so early and don't
> > > update it.  If you don't need it above for can_move_early_debug_stmts, can
> > > you just do it back where it used to be done,
> > 
> > Here it is everything in patch form, in case some volunteers are willing to
> > test it on their targets, because we need faster turn-arounds for this.
> > 
> > 2017-12-13  Alexandre Oliva <aoliva@redhat.com>
> > 	    Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR bootstrap/83396
> > 	PR debug/83391
> > 	* tree-cfgcleanup.c (remove_forwarder_block): Keep after
> > 	labels debug stmts that can only appear after labels.
> > 
> > 	* gcc.dg/torture/pr83396.c: New test.
> > 	* g++.dg/torture/pr83391.C: New test.
> 
> Bootstrapped/regtested on x86_64-linux, i686-linux and powerpc64le-linux,
> powerpc64-linux regtest still pending, ok for trunk?

Ok.

Richard.

> > --- gcc/tree-cfgcleanup.c.jj	2017-12-12 09:48:26.813393301 +0100
> > +++ gcc/tree-cfgcleanup.c	2017-12-13 11:39:03.373065381 +0100
> > @@ -536,9 +536,14 @@ remove_forwarder_block (basic_block bb)
> >       defined labels and labels with an EH landing pad number to the
> >       new block, so that the redirection of the abnormal edges works,
> >       jump targets end up in a sane place and debug information for
> > -     labels is retained.  */
> > +     labels is retained.
> > +
> > +     While at that, move any debug stmts that appear before or in between
> > +     labels, but not those that can only appear after labels.  */
> >    gsi_to = gsi_start_bb (dest);
> > -  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> > +  gsi = gsi_start_bb (bb);
> > +  gimple_stmt_iterator gsie = gsi_after_labels (bb);
> > +  while (gsi_stmt (gsi) != gsi_stmt (gsie))
> >      {
> >        tree decl;
> >        label = gsi_stmt (gsi);
> > @@ -557,6 +562,21 @@ remove_forwarder_block (basic_block bb)
> >  	gsi_next (&gsi);
> >      }
> >  
> > +  /* Move debug statements if the destination has a single predecessor.  */
> > +  if (can_move_debug_stmts && !gsi_end_p (gsi))
> > +    {
> > +      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
> > +      gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
> > +      do
> > +	{
> > +	  gimple *debug = gsi_stmt (gsi);
> > +	  gcc_assert (is_gimple_debug (debug));
> > +	  gsi_remove (&gsi, false);
> > +	  gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
> > +	}
> > +      while (!gsi_end_p (gsi));
> > +    }
> > +
> >    bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
> >  
> >    /* Update the dominators.  */
> > --- gcc/testsuite/g++.dg/torture/pr83391.C.jj
> > +++ gcc/testsuite/g++.dg/torture/pr83391.C
> > @@ -0,0 +1,36 @@
> > +// PR debug/83391
> > +// { dg-do compile }
> > +// { dg-options "-g" }
> > +// { dg-additional-options "-mbranch-cost=1" { target { i?86-*-* x86_64-*-* mips*-*-* s390*-*-* avr*-*-* } } }
> > +
> > +unsigned char a;
> > +enum E { F, G, H } b;
> > +int c, d;
> > +
> > +void
> > +foo ()
> > +{
> > +  int e;
> > +  bool f;
> > +  E g = b;
> > +  while (1)
> > +    {
> > +      unsigned char h = a ? d : 0;
> > +      switch (g)
> > +	{
> > +	case 0:
> > +	  f = h <= 'Z' || h >= 'a' && h <= 'z';
> > +	  break;
> > +	case 1:
> > +	  {
> > +	    unsigned char i = h;
> > +	    e = 0;
> > +	  }
> > +	  if (e || h)
> > +	    g = H;
> > +	  /* FALLTHRU */
> > +	default:
> > +	  c = 0;
> > +	}
> > +    }
> > +}
> > --- gcc/testsuite/gcc.dg/torture/pr83396.c.jj
> > +++ gcc/testsuite/gcc.dg/torture/pr83396.c
> > @@ -0,0 +1,38 @@
> > +/* PR bootstrap/83396 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-g" } */
> > +
> > +int fn1 (void);
> > +void fn2 (void *, const char *);
> > +void fn3 (void);
> > +
> > +void
> > +fn4 (long long x)
> > +{
> > +  fn3 ();
> > +}
> > +
> > +void
> > +fn5 (long long x)
> > +{
> > +  if (x)
> > +    fn3();
> > +}
> > +
> > +void
> > +fn6 (long long x)
> > +{
> > +  switch (fn1 ())
> > +    {
> > +    case 0:
> > +      fn5 (x);
> > +    case 2:
> > +      fn2 (0, "");
> > +      break;
> > +    case 1:
> > +    case 3:
> > +      fn4(x);
> > +    case 5:
> > +      fn2 (0, "");
> > +    }
> > +}
> > 
> > 
> > 	Jakub
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [SFN] Bootstrap broken

[Christophe Lyon]

On 13 December 2017 at 11:45, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 13, 2017 at 10:28:22AM +0100, Jakub Jelinek wrote:
>> Formatting, this should be
>>   bool can_move_early_debug_stmts
>>     = ...
>> and the line is too long, so needs to be wrapped.
>>
>> Furthermore, I must say I don't understand why
>> can_move_early_debug_stmts should care whether there are any labels in
>> dest bb or not.  That sounds very risky for introducing non-# DEBUG BEGIN_STMT
>> debug insns before labels if it could happen.  Though, if
>> gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
>> do anything and nothing cares about can_move_early_debug_stmts afterwards.
>> So, in short, can_move_early_debug_stmts is used only if
>> gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
>> can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);
>>
>> So, can we get rid of can_move_early_debug_stmts altogether and just use
>> can_move_debug_stmts in there instead?
>>
>> Another thing I find risky is that you compute gsie_to so early and don't
>> update it.  If you don't need it above for can_move_early_debug_stmts, can
>> you just do it back where it used to be done,
>
> Here it is everything in patch form, in case some volunteers are willing to
> test it on their targets, because we need faster turn-arounds for this.
>

Thanks for that, it certainly helps.

So, this version does restore a successful build on arm --with-mode=thumb,
but pr69102 still fails in arm mode.

As I mentioned in PR83396, the 4 patches attached there fix all the
problems I noticed.

HTH.

Christophe

> 2017-12-13  Alexandre Oliva <aoliva@redhat.com>
>             Jakub Jelinek  <jakub@redhat.com>
>
>         PR bootstrap/83396
>         PR debug/83391
>         * tree-cfgcleanup.c (remove_forwarder_block): Keep after
>         labels debug stmts that can only appear after labels.
>
>         * gcc.dg/torture/pr83396.c: New test.
>         * g++.dg/torture/pr83391.C: New test.
>
> --- gcc/tree-cfgcleanup.c.jj    2017-12-12 09:48:26.813393301 +0100
> +++ gcc/tree-cfgcleanup.c       2017-12-13 11:39:03.373065381 +0100
> @@ -536,9 +536,14 @@ remove_forwarder_block (basic_block bb)
>       defined labels and labels with an EH landing pad number to the
>       new block, so that the redirection of the abnormal edges works,
>       jump targets end up in a sane place and debug information for
> -     labels is retained.  */
> +     labels is retained.
> +
> +     While at that, move any debug stmts that appear before or in between
> +     labels, but not those that can only appear after labels.  */
>    gsi_to = gsi_start_bb (dest);
> -  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> +  gsi = gsi_start_bb (bb);
> +  gimple_stmt_iterator gsie = gsi_after_labels (bb);
> +  while (gsi_stmt (gsi) != gsi_stmt (gsie))
>      {
>        tree decl;
>        label = gsi_stmt (gsi);
> @@ -557,6 +562,21 @@ remove_forwarder_block (basic_block bb)
>         gsi_next (&gsi);
>      }
>
> +  /* Move debug statements if the destination has a single predecessor.  */
> +  if (can_move_debug_stmts && !gsi_end_p (gsi))
> +    {
> +      gcc_assert (gsi_stmt (gsi) == gsi_stmt (gsie));
> +      gimple_stmt_iterator gsie_to = gsi_after_labels (dest);
> +      do
> +       {
> +         gimple *debug = gsi_stmt (gsi);
> +         gcc_assert (is_gimple_debug (debug));
> +         gsi_remove (&gsi, false);
> +         gsi_insert_before (&gsie_to, debug, GSI_SAME_STMT);
> +       }
> +      while (!gsi_end_p (gsi));
> +    }
> +
>    bitmap_set_bit (cfgcleanup_altered_bbs, dest->index);
>
>    /* Update the dominators.  */
> --- gcc/testsuite/g++.dg/torture/pr83391.C.jj
> +++ gcc/testsuite/g++.dg/torture/pr83391.C
> @@ -0,0 +1,36 @@
> +// PR debug/83391
> +// { dg-do compile }
> +// { dg-options "-g" }
> +// { dg-additional-options "-mbranch-cost=1" { target { i?86-*-* x86_64-*-* mips*-*-* s390*-*-* avr*-*-* } } }
> +
> +unsigned char a;
> +enum E { F, G, H } b;
> +int c, d;
> +
> +void
> +foo ()
> +{
> +  int e;
> +  bool f;
> +  E g = b;
> +  while (1)
> +    {
> +      unsigned char h = a ? d : 0;
> +      switch (g)
> +       {
> +       case 0:
> +         f = h <= 'Z' || h >= 'a' && h <= 'z';
> +         break;
> +       case 1:
> +         {
> +           unsigned char i = h;
> +           e = 0;
> +         }
> +         if (e || h)
> +           g = H;
> +         /* FALLTHRU */
> +       default:
> +         c = 0;
> +       }
> +    }
> +}
> --- gcc/testsuite/gcc.dg/torture/pr83396.c.jj
> +++ gcc/testsuite/gcc.dg/torture/pr83396.c
> @@ -0,0 +1,38 @@
> +/* PR bootstrap/83396 */
> +/* { dg-do compile } */
> +/* { dg-options "-g" } */
> +
> +int fn1 (void);
> +void fn2 (void *, const char *);
> +void fn3 (void);
> +
> +void
> +fn4 (long long x)
> +{
> +  fn3 ();
> +}
> +
> +void
> +fn5 (long long x)
> +{
> +  if (x)
> +    fn3();
> +}
> +
> +void
> +fn6 (long long x)
> +{
> +  switch (fn1 ())
> +    {
> +    case 0:
> +      fn5 (x);
> +    case 2:
> +      fn2 (0, "");
> +      break;
> +    case 1:
> +    case 3:
> +      fn4(x);
> +    case 5:
> +      fn2 (0, "");
> +    }
> +}
>
>
>         Jakub

Re: [SFN] Bootstrap broken

[Alexandre Oliva]

Jakub, I summed up to you yesterday on IRC what I expand below; this is
just for the public record.

On Dec 13, 2017, Jakub Jelinek <jakub@redhat.com> wrote:

> Furthermore, I must say I don't understand why
> can_move_early_debug_stmts should care whether there are any labels in
> dest bb or not.

An earlier attempt tried to move all labels and debug stmts in a single
loop, and started with this early debug setting and inserting at the
block start stmt, and later switched to the preexisting debug setting
and inserting at the after-label stmt.  In the end, I decided this was
trickier and riskier than two separate loops, but failed to simplify the
logic back all the way.

> Though, if
> gsi_stmt (gsi) == gsi_stmt (gsie), then the loop right below it will not
> do anything and nothing cares about can_move_early_debug_stmts afterwards.
> So, in short, can_move_early_debug_stmts is used only if
> gsi_stmt (gsi) != gsi_stmt (gsie), and therefore
> can_move_early_debug_stmts if it is used is can_move_debug_stmts && (1 || ...);

*nod*

> So, can we get rid of can_move_early_debug_stmts altogether and just use
> can_move_debug_stmts in there instead?

Yes.

> Another thing I find risky is that you compute gsie_to so early and don't
> update it.

The point of computing it early was *precisely* to preserve the
insertion point should we have both before-label and after-label debug
stmts to move.  But in the end it doesn't matter: we'll either find the
right spot after moving labels, or we'll be at it if there weren't any
labels to move.


Thanks for the improvements!

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer