Avoid creating unbounded complexity debug insns in valtrack (PR debug/77844, take 2)

On Tue, Dec 13, 2016 at 10:05:58AM +0100, Richard Biener wrote:
> Hmm, so the "easier" fix would be to always create a debug insn
> right after INSN setting a new debug reg to SRC and then doing the
> replacement with the new debug reg?  With the "optimization"
> to not do that for some single use and/or "simple" SRC cases
> (I think "simple" SRC would be sth with a single/zero register reference).
> 
> You are fixing it at the wrong end IMHO, by throwing away the
> thing after propagating (and doing all the work and doing the
> check on each individual propagation result).

Here is updated patch that does that.  I had to change the combiner
in a couple of places, because combiner is very unhappy if new instructions
(with uids above the highest) are inserted into the insn stream,
e.g. FOR_EACH_LOG_LINK (link, temp_insn) for DEBUG_INSN temp_insn
will then crash.  But given that DEBUG_INSNs never have non-NULL LOG_LINKS,
I think we can just avoid using LOG_LINKS or INSN_COST on DEBUG_INSNs
and allow new DEBUG_INSNs be inserted into the stream (but not regular
insns - normally combine just reuses INSN_UID of the insns it is replacing).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-12-14  Jakub Jelinek  <jakub@redhat.com>

	PR debug/77844
	* valtrack.c: Include rtl-iter.h.
	(struct rtx_subst_pair): Add insn field.
	(propagate_for_debug_subst): If pair->to contains at least 2
	regs, create a DEBUG_INSN with a debug temp before pair->insn
	and replace from with the debug temp instead of pair->to.
	(propagate_for_debug): Initialize p.insn.
	* combine.c (find_single_use, combine_instructions,
	cant_combine_insn_p, try_combine): Use NONDEBUG_INSN_P instead of
	INSN_P.

	* g++.dg/opt/pr77844.C: New test.



	Jakub

On Wed, 14 Dec 2016, Jakub Jelinek wrote:

> On Tue, Dec 13, 2016 at 10:05:58AM +0100, Richard Biener wrote:
> > Hmm, so the "easier" fix would be to always create a debug insn
> > right after INSN setting a new debug reg to SRC and then doing the
> > replacement with the new debug reg?  With the "optimization"
> > to not do that for some single use and/or "simple" SRC cases
> > (I think "simple" SRC would be sth with a single/zero register reference).
> > 
> > You are fixing it at the wrong end IMHO, by throwing away the
> > thing after propagating (and doing all the work and doing the
> > check on each individual propagation result).
> 
> Here is updated patch that does that.  I had to change the combiner
> in a couple of places, because combiner is very unhappy if new instructions
> (with uids above the highest) are inserted into the insn stream,
> e.g. FOR_EACH_LOG_LINK (link, temp_insn) for DEBUG_INSN temp_insn
> will then crash.  But given that DEBUG_INSNs never have non-NULL LOG_LINKS,
> I think we can just avoid using LOG_LINKS or INSN_COST on DEBUG_INSNs
> and allow new DEBUG_INSNs be inserted into the stream (but not regular
> insns - normally combine just reuses INSN_UID of the insns it is replacing).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

LGTM, please leave others some time to comment though.

Thanks,
Richard.

> 2016-12-14  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/77844
> 	* valtrack.c: Include rtl-iter.h.
> 	(struct rtx_subst_pair): Add insn field.
> 	(propagate_for_debug_subst): If pair->to contains at least 2
> 	regs, create a DEBUG_INSN with a debug temp before pair->insn
> 	and replace from with the debug temp instead of pair->to.
> 	(propagate_for_debug): Initialize p.insn.
> 	* combine.c (find_single_use, combine_instructions,
> 	cant_combine_insn_p, try_combine): Use NONDEBUG_INSN_P instead of
> 	INSN_P.
> 
> 	* g++.dg/opt/pr77844.C: New test.
> 
> --- gcc/valtrack.c.jj	2016-12-12 22:51:13.590952439 +0100
> +++ gcc/valtrack.c	2016-12-13 12:11:16.436894296 +0100
> @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.
>  #include "regs.h"
>  #include "memmodel.h"
>  #include "emit-rtl.h"
> +#include "rtl-iter.h"
>  
>  /* gen_lowpart_no_emit hook implementation for DEBUG_INSNs.  In DEBUG_INSNs,
>     all lowpart SUBREGs are valid, despite what the machine requires for
> @@ -145,6 +146,7 @@ struct rtx_subst_pair
>  {
>    rtx to;
>    bool adjusted;
> +  rtx_insn *insn;
>  };
>  
>  /* DATA points to an rtx_subst_pair.  Return the value that should be
> @@ -162,6 +164,23 @@ propagate_for_debug_subst (rtx from, con
>        pair->adjusted = true;
>        pair->to = cleanup_auto_inc_dec (pair->to, VOIDmode);
>        pair->to = make_compound_operation (pair->to, SET);
> +      /* Avoid propagation from growing DEBUG_INSN expressions too much.  */
> +      int cnt = 0;
> +      subrtx_iterator::array_type array;
> +      FOR_EACH_SUBRTX (iter, array, pair->to, ALL)
> +	if (REG_P (*iter) && ++cnt > 1)
> +	  {
> +	    rtx dval = make_debug_expr_from_rtl (old_rtx);
> +	    /* Emit a debug bind insn.  */
> +	    rtx bind
> +	      = gen_rtx_VAR_LOCATION (GET_MODE (old_rtx),
> +				      DEBUG_EXPR_TREE_DECL (dval), pair->to,
> +				      VAR_INIT_STATUS_INITIALIZED);
> +	    rtx_insn *bind_insn = emit_debug_insn_before (bind, pair->insn);
> +	    df_insn_rescan (bind_insn);
> +	    pair->to = dval;
> +	    break;
> +	  }
>        return pair->to;
>      }
>    return copy_rtx (pair->to);
> @@ -182,6 +201,7 @@ propagate_for_debug (rtx_insn *insn, rtx
>    struct rtx_subst_pair p;
>    p.to = src;
>    p.adjusted = false;
> +  p.insn = NEXT_INSN (insn);
>  
>    next = NEXT_INSN (insn);
>    last = NEXT_INSN (last);
> --- gcc/combine.c.jj	2016-12-13 18:07:44.716123373 +0100
> +++ gcc/combine.c	2016-12-13 20:34:07.645287571 +0100
> @@ -676,7 +676,7 @@ find_single_use (rtx dest, rtx_insn *ins
>    for (next = NEXT_INSN (insn);
>         next && BLOCK_FOR_INSN (next) == bb;
>         next = NEXT_INSN (next))
> -    if (INSN_P (next) && dead_or_set_p (next, dest))
> +    if (NONDEBUG_INSN_P (next) && dead_or_set_p (next, dest))
>        {
>  	FOR_EACH_LOG_LINK (link, next)
>  	  if (link->insn == insn && link->regno == REGNO (dest))
> @@ -1127,7 +1127,7 @@ combine_instructions (rtx_insn *f, unsig
>  
>    int new_direct_jump_p = 0;
>  
> -  for (first = f; first && !INSN_P (first); )
> +  for (first = f; first && !NONDEBUG_INSN_P (first); )
>      first = NEXT_INSN (first);
>    if (!first)
>      return 0;
> @@ -2275,7 +2275,7 @@ cant_combine_insn_p (rtx_insn *insn)
>    /* If this isn't really an insn, we can't do anything.
>       This can occur when flow deletes an insn that it has merged into an
>       auto-increment address.  */
> -  if (! INSN_P (insn))
> +  if (! NONDEBUG_INSN_P (insn))
>      return 1;
>  
>    /* Never combine loads and stores involving hard regs that are likely
> @@ -4178,7 +4178,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
>  		    || insn != BB_HEAD (this_basic_block->next_bb));
>  	   insn = NEXT_INSN (insn))
>  	{
> -	  if (INSN_P (insn) && reg_referenced_p (ni2dest, PATTERN (insn)))
> +	  if (NONDEBUG_INSN_P (insn)
> +	      && reg_referenced_p (ni2dest, PATTERN (insn)))
>  	    {
>  	      FOR_EACH_LOG_LINK (link, insn)
>  		if (link->insn == i3)
> @@ -4319,9 +4320,9 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
>  	    for (temp_insn = NEXT_INSN (i2);
>  		 temp_insn
>  		 && (this_basic_block->next_bb == EXIT_BLOCK_PTR_FOR_FN (cfun)
> -			  || BB_HEAD (this_basic_block) != temp_insn);
> +		     || BB_HEAD (this_basic_block) != temp_insn);
>  		 temp_insn = NEXT_INSN (temp_insn))
> -	      if (temp_insn != i3 && INSN_P (temp_insn))
> +	      if (temp_insn != i3 && NONDEBUG_INSN_P (temp_insn))
>  		FOR_EACH_LOG_LINK (link, temp_insn)
>  		  if (link->insn == i2)
>  		    link->insn = i3;
> --- gcc/testsuite/g++.dg/opt/pr77844.C.jj	2016-12-13 11:52:12.355311153 +0100
> +++ gcc/testsuite/g++.dg/opt/pr77844.C	2016-12-13 11:52:12.355311153 +0100
> @@ -0,0 +1,32 @@
> +// PR debug/77844
> +// { dg-do compile }
> +// { dg-options "-O3 -g" }
> +
> +#include <vector>
> +
> +void
> +foo (std::vector<bool>& v, int i, int j)
> +{
> +  for (int k = 0; k < 5; ++k)
> +    v[5 * i + k] = v[5 * i + k] | v[5 * j + k];
> +}
> +
> +void
> +bar (std::vector<bool>& v, int i, int j)
> +{
> +  for (int k = 0; k < 5; ++k)
> +    v[5 * i + k] = v[5 * i + k] ^ v[5 * j + k];
> +}
> +
> +void
> +baz (std::vector<bool>& v)
> +{
> +  foo (v, 4, 1);
> +  foo (v, 4, 2);
> +  foo (v, 4, 3);
> +  foo (v, 4, 4);
> +  bar (v, 4, 1);
> +  bar (v, 4, 2);
> +  bar (v, 4, 3);
> +  bar (v, 4, 4);
> +}
> 
> 
> 	Jakub
> 
>

On Wed, Dec 14, 2016 at 09:34:44AM +0100, Jakub Jelinek wrote:
> Here is updated patch that does that.  I had to change the combiner
> in a couple of places, because combiner is very unhappy if new instructions
> (with uids above the highest) are inserted into the insn stream,
> e.g. FOR_EACH_LOG_LINK (link, temp_insn) for DEBUG_INSN temp_insn
> will then crash.  But given that DEBUG_INSNs never have non-NULL LOG_LINKS,
> I think we can just avoid using LOG_LINKS or INSN_COST on DEBUG_INSNs
> and allow new DEBUG_INSNs be inserted into the stream (but not regular
> insns - normally combine just reuses INSN_UID of the insns it is replacing).

Could you do a  gcc_checking_assert (INSN_UID (INSN) <= max_uid_known)
in the LOG_LINKS and INSN_COST defines?  It is hard to check if you
caught every use.

> @@ -2275,7 +2275,7 @@ cant_combine_insn_p (rtx_insn *insn)
>    /* If this isn't really an insn, we can't do anything.
>       This can occur when flow deletes an insn that it has merged into an
>       auto-increment address.  */
> -  if (! INSN_P (insn))
> +  if (! NONDEBUG_INSN_P (insn))
>      return 1;

Please get rid of the extra space (and thanks for the other formatting
fixes, little by little :-) )

Looks fine otherwise.  Thanks,


Segher