Re: [PATCH] Remove forwarder blocks in tree-cfgcleanup even at -O0 if they don't change locations (PR debug/44205)

[Richard Guenther <rguenther at suse dot de>]

On Fri, 21 May 2010, Jakub Jelinek wrote:

> Hi!
> 
> The testcase from the PR has wrong line information at -O0.
> When we have if (x) { if (y) { block1; } else { block2; } } else { block3; } block4;
> then the next location after block1 lines should be block4, rather than
> block2.  The gimplifier creates in this case
> ... block1; goto cont_label_after_block2; block2; cont_label_after_block2: goto cont_label_after_block3; ...
> where the gotos intentionally don't have any locus, only
> block{1,2,3,4} stmts and the condition stmts have:
>               g = gimple_build_goto (label_cont);
>     
>               /* GIMPLE_COND's are very low level; they have embedded
>                  gotos.  This particular embedded goto should not be marked
>                  with the location of the original COND_EXPR, as it would
>                  correspond to the COND_EXPR's condition, not the ELSE or
>                  the
>                  THEN arms.  To avoid marking it with the wrong location,
>                  flag
>                  it as "no location".  */
>               gimple_set_do_not_emit_location (g);
> but it would be very hard at gimplify time to ensure that block1's end goto
> jumps to cont lable after block3 instead after block1.
> In tree-cfgcleanup we then refuse to remove the forwarder block after
> block2:
>   /* Forwarder blocks can carry line number information which is
>      useful when debugging, so we only clean them up when
>      optimizing.  */
>   if (optimize > 0
>       && tree_forwarder_block_p (bb, false)
>       && remove_forwarder_block (bb))
>     return true;
> At RTL expansion time, the jumps which previously had no locus get one
> from the preceeding insns and then in cfglayout mode finally the forwarder
> block is removed and the jump that is kept has line number of last stmt
> in block2.
> 
> The following patch fixes it by enabling removal of forwarder blocks even
> at -O0, as long as they don't affect line numbers (locus of all
> incoming/outgoing edges and labels is the same).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux and Jan Kratochvil
> regtested it on gdb testsuite.  Ok for trunk?

Ok.  Is this a regression in 4.5 as we removed the useless pass
which did some of the jump optimizations in 4.4?

Thanks,
Richard.

> 2010-05-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/44205
> 	* tree-cfgcleanup.c (tree_forwarder_block_p): Return false if
> 	at -O0 goto_locus of any of the incoming edges differs from
> 	goto_locus of outgoing edge, or gimple_location of any of the
> 	labels differs.
> 
> --- gcc/tree-cfgcleanup.c.jj	2010-04-05 18:48:59.000000000 +0200
> +++ gcc/tree-cfgcleanup.c	2010-05-20 16:29:42.000000000 +0200
> @@ -267,6 +267,7 @@ static bool
>  tree_forwarder_block_p (basic_block bb, bool phi_wanted)
>  {
>    gimple_stmt_iterator gsi;
> +  location_t locus;
>  
>    /* BB must have a single outgoing edge.  */
>    if (single_succ_p (bb) != 1
> @@ -285,6 +286,8 @@ tree_forwarder_block_p (basic_block bb, 
>    gcc_assert (bb != ENTRY_BLOCK_PTR);
>  #endif
>  
> +  locus = single_succ_edge (bb)->goto_locus;
> +
>    /* There should not be an edge coming from entry, or an EH edge.  */
>    {
>      edge_iterator ei;
> @@ -293,6 +296,10 @@ tree_forwarder_block_p (basic_block bb, 
>      FOR_EACH_EDGE (e, ei, bb->preds)
>        if (e->src == ENTRY_BLOCK_PTR || (e->flags & EDGE_EH))
>  	return false;
> +      /* If goto_locus of any of the edges differs, prevent removing
> +	 the forwarder block for -O0.  */
> +      else if (optimize == 0 && e->goto_locus != locus)
> +	return false;
>    }
>  
>    /* Now walk through the statements backward.  We can ignore labels,
> @@ -306,6 +313,8 @@ tree_forwarder_block_p (basic_block bb, 
>  	case GIMPLE_LABEL:
>  	  if (DECL_NONLOCAL (gimple_label_label (stmt)))
>  	    return false;
> +	  if (optimize == 0 && gimple_location (stmt) != locus)
> +	    return false;
>  	  break;
>  
>  	  /* ??? For now, hope there's a corresponding debug
> @@ -608,11 +617,7 @@ cleanup_tree_cfg_bb (basic_block bb)
>  
>    retval = cleanup_control_flow_bb (bb);
>  
> -  /* Forwarder blocks can carry line number information which is
> -     useful when debugging, so we only clean them up when
> -     optimizing.  */
> -  if (optimize > 0
> -      && tree_forwarder_block_p (bb, false)
> +  if (tree_forwarder_block_p (bb, false)
>        && remove_forwarder_block (bb))
>      return true;
>  
> 
> 	Jakub
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex