Improve debug info if tree DCE removes stores (PR debug/50317)

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.
	* cfgexpand.c (expand_gimple_basic_block): Don't emit DEBUG_INSNs
	for !target_for_debug_bind variables.


	Jakub

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/01/11 09:49, 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. * cfgexpand.c
> (expand_gimple_basic_block): Don't emit DEBUG_INSNs for
> !target_for_debug_bind variables.
OK.
Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJO17rmAAoJEBRtltQi2kC7q8EH/1VnjCdoj8E59TJENYmD0Kx9
dP1zeN8uxlD0goVoSum8FbYArL4cbaajPgA+I1+hlGIBK2htl+fSwKiKaq7i6wNT
r21GL27JVtGdPNjDh37Srb3DvgmtKBCP9iSqZDjaO8xAB0zjnoTsYwlx3EfdWi8C
aSuJsAEqAshaU+GptWzG0CsXe5R+XQB10c+RGP5MFpsRpzWVHPXFOZ6yUmFF0EOO
4MI5eHM+C5j0W4NigpZL18YAjEQBsH+ricgO6xjg/XAU3ro5hOidlno4F71+6wCO
q24Mr8RMTpEe+TZE18GLx9iNrIUShDXTqjAwt1fvYVxi96hGgwAyU4BywruXNII=
=Eg48
-----END PGP SIGNATURE-----

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

Hi,

On Fri, 2 Dec 2011, Richard Guenther wrote:

> > 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]

Why would it be invalid?  It's meaningful to talk about a full object and 
references to it even without having its address taken.  Normal 
loads/stores do the same.

> once you update that stmt with update_stmt you'll get an SSA operand
> for transfer.0

That's the thing which should be fixed then.

> Why do this in remove_unused_locals and not in update_address_taken?

Another walk over all statements just for this?  Meh.

> Or, why do it at all?

The debug machinery seems to be unhappy about variables that are address 
taken.

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


Ciao,
Michael.