Re: [PATCH] Improve debug info if tree DCE removes stores (PR debug/50317)

[Richard Guenther <rguenther at suse dot de>]

On Thu, 1 Dec 2011, Jakub Jelinek wrote:

> Hi!
> 
> As discussed in the PR, in 4.7 we regressed some GDB testcases, because
> previously unused addressable vars were first previously optimized into
> non-addressable and only afterwards removed (which results in correct debug
> stmts covering those assignments), but after some recent changes it is
> CDDCE that removes them before they are update_address_taken optimized.
> 
> In the PR I've offered a patch to schedule another update_address_taken
> pass before first cddce, but Michael is right that perhaps some other
> DCE pass could have similar issue.
> 
> So this patch instead, if the DCEd var stores have addressable lhs, but
> with is_gimple_reg_type types, we add debug stmts even for them.
> Such variables aren't target_for_debug_bind though, which breaks
> var-tracking.  So, the patch if all occurrences of the var are optimized
> away, just clears TREE_ADDRESSABLE bit like update_address_taken would,
> and, if that didn't happen until expansion, just ignores those debug
> stmts so that var-tracking isn't upset by seing non-tracked vars in debug
> insns.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2011-12-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/50317
> 	* tree-ssa-dce.c (remove_dead_stmt): Add a debug stmt when removing
> 	as unnecessary a store to a variable with gimple reg type.
> 	* tree-ssa-live.c (remove_unused_locals): Clear TREE_ADDRESSABLE bit
> 	on local unreferenced variables.

This change seems wrong.  We are turning valid gimple

# DEBUG D#2 => transfer.0  [with addres taken]

into invalid one

# DEBUG D#2 => transfer.0  [without address taken]

once you update that stmt with update_stmt you'll get an SSA operand
for transfer.0 which is not in SSA form because you fail to rewrite it
into.

Why do this in remove_unused_locals and not in update_address_taken?
Or, why do it at all?

I have a SSA operand checking patch that catches this now ...

Thanks,
Richard.


> 	* cfgexpand.c (expand_gimple_basic_block): Don't emit DEBUG_INSNs
> 	for !target_for_debug_bind variables.
> 
> --- gcc/tree-ssa-live.c.jj	2011-11-28 15:41:46.376749700 +0100
> +++ gcc/tree-ssa-live.c	2011-12-01 12:04:12.920595572 +0100
> @@ -1,5 +1,5 @@
>  /* Liveness for SSA trees.
> -   Copyright (C) 2003, 2004, 2005, 2007, 2008, 2009, 2010
> +   Copyright (C) 2003, 2004, 2005, 2007, 2008, 2009, 2010, 2011
>     Free Software Foundation, Inc.
>     Contributed by Andrew MacLeod <amacleod@redhat.com>
>  
> @@ -814,7 +814,15 @@ remove_unused_locals (void)
>  	      bitmap_set_bit (global_unused_vars, DECL_UID (var));
>  	    }
>  	  else
> -	    continue;
> +	    {
> +	      /* For unreferenced local vars drop TREE_ADDRESSABLE
> +		 bit in case it is referenced from debug stmts.  */
> +	      if (DECL_CONTEXT (var) == current_function_decl
> +		  && TREE_ADDRESSABLE (var)
> +		  && is_gimple_reg_type (TREE_TYPE (var)))
> +		TREE_ADDRESSABLE (var) = 0;
> +	      continue;
> +	    }
>  	}
>        else if (TREE_CODE (var) == VAR_DECL
>  	       && DECL_HARD_REGISTER (var)
> --- gcc/tree-ssa-dce.c.jj	2011-11-28 15:41:46.376749700 +0100
> +++ gcc/tree-ssa-dce.c	2011-12-01 12:04:12.920595572 +0100
> @@ -1215,6 +1215,26 @@ remove_dead_stmt (gimple_stmt_iterator *
>  	  ei_next (&ei);
>      }
>  
> +  /* If this is a store into a variable that is being optimized away,
> +     add a debug bind stmt if possible.  */
> +  if (MAY_HAVE_DEBUG_STMTS
> +      && gimple_assign_single_p (stmt)
> +      && is_gimple_val (gimple_assign_rhs1 (stmt)))
> +    {
> +      tree lhs = gimple_assign_lhs (stmt);
> +      if ((TREE_CODE (lhs) == VAR_DECL || TREE_CODE (lhs) == PARM_DECL)
> +	  && !DECL_IGNORED_P (lhs)
> +	  && is_gimple_reg_type (TREE_TYPE (lhs))
> +	  && !is_global_var (lhs)
> +	  && !DECL_HAS_VALUE_EXPR_P (lhs))
> +	{
> +	  tree rhs = gimple_assign_rhs1 (stmt);
> +	  gimple note
> +	    = gimple_build_debug_bind (lhs, unshare_expr (rhs), stmt);
> +	  gsi_insert_after (i, note, GSI_SAME_STMT);
> +	}
> +    }
> +
>    unlink_stmt_vdef (stmt);
>    gsi_remove (i, true);
>    release_defs (stmt);
> --- gcc/cfgexpand.c.jj	2011-12-01 11:44:56.156345109 +0100
> +++ gcc/cfgexpand.c	2011-12-01 12:37:57.764791257 +0100
> @@ -3903,6 +3903,11 @@ expand_gimple_basic_block (basic_block b
>  	      rtx val;
>  	      enum machine_mode mode;
>  
> +	      if (TREE_CODE (var) != DEBUG_EXPR_DECL
> +		  && TREE_CODE (var) != LABEL_DECL
> +		  && !target_for_debug_bind (var))
> +		goto delink_debug_stmt;
> +
>  	      if (gimple_debug_bind_has_value_p (stmt))
>  		value = gimple_debug_bind_get_value (stmt);
>  	      else
> @@ -3932,6 +3937,7 @@ expand_gimple_basic_block (basic_block b
>  		  PAT_VAR_LOCATION_LOC (val) = (rtx)value;
>  		}
>  
> +	    delink_debug_stmt:
>  	      /* In order not to generate too many debug temporaries,
>  	         we delink all uses of debug statements we already expanded.
>  		 Therefore debug statements between definition and real
> 
> 	Jakub
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer