Re: [PATCH] Fix find_inc in the scheduler (take 2, PR target/62025)

[Vladimir Makarov <vmakarov at redhat dot com>]

On 2014-08-13, 6:30 PM, Jakub Jelinek wrote:
> On Tue, Aug 12, 2014 at 05:12:41PM -0400, Vladimir Makarov wrote:
> > On 08/12/2014 03:35 PM, Jakub Jelinek wrote:
> > > Hi!
> > >
> > > As detailed in the PR, find_inc ignored any possible clobbers on
> > > inc_insn (typically %cc/flags/etc. register) and thus we could ignore
> > > all register dependencies between mem_insn and inc_insn even when
> > > we could only safely ignore the mem_reg0 register dependency.
> > >
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > > Is slightly modified version (just for different iteration over defs and
> > > uses) ok for 4.9?
> > Ok.  Thanks for fixing this PR, Jakub.
>
> Unfortunately, after looking at the code some more, there are further issues
> (theoretical only, don't have testcases).
>
> Here is an incremental patch that I'm bootstrapping/regtesting with
> additional instrumentation.
>
> Apparently find_inc handles both the cases where the increment can initially
> dominate the mem insn or vice versa.
>
> My patch from yesterday was trying to fix the case where we have:
>
> mem_insn uses %rX in memory address, uses %flags
> ...
> inc_insn %rX += const, clobbers %flags
>
> where it is wrong to move inc_insn before mem_insn, because if it is moved
> in between %flags setter before mem_insn and mem_insn, %flags might have
> wrong value.
>
> I think giving up for this if mem_insn is dominated originally by inc_insn
> is unnecessary, there should be a dependency between %flags setter and
> inc_insn which should prevent the move.
>
> find_inc checks that inc_insn is single_set doing %rX += const or
> %rX = %rY + const, so the only defs other than %rX IMHO must be clobbers.
>
> But what I see as a problematic case (in my statistics dumps) is:
>
> inc_insn %rX += const, clobbers %flags
> ...
> mem_insn uses %rX in memory address, sets %flags
>
> In this case, moving inc_insn after mem_insn is wrong, if inc_insn is moved
> in between mem_insn and the user of %flags after it.  This (potential)
> wrong-code I've seen 9611 times in 32-bit code and so far 766 times in
> 64-bit code during the two bootstraps/regtests.
>
> The case when both inc_insn and mem_insn both clobber the same reg (seems to
> happen fairly often, with %flags) is IMHO not a problem.
>
> Also, we've discussed with Vlad on IRC the (theoretical?) possibility that
> inc_insn could have also USE rtxs in the pattern beyond the single set and
> possible clobbers.  In that case, we don't want to move it either way if
> mem_insn defines the reg used in the USE.  Note, no cases found in
> x86_64-linux and i686-linux bootstrap statistics dumps.
>
> So, does this make sense?

What a brain damaged excercise to analyze cases for the patch especially 
at the end of day:)  But I believe the patch is ok.

Only minor thing.  Please, read my comments below.

> 2014-08-14  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/62025
> 	* sched-deps.c (find_inc): Limit the test for inc_insn defs
> 	vs. mem_insn uses to !backwards case only.  Give up also if
> 	any mem_insn def is used by inc_insn or if non-clobber
> 	mem_insn def in backwards case is clobbered by inc_insn.
>
> --- gcc/sched-deps.c.jj	2014-08-12 17:06:26.000000000 +0200
> +++ gcc/sched-deps.c	2014-08-14 00:09:38.000000000 +0200
> @@ -4751,24 +4751,54 @@ find_inc (struct mem_inc_info *mii, bool
>   			   "inc conflicts with store failure.\n");
>   		goto next;
>   	      }
> -
> -	  /* The inc instruction could have clobbers, make sure those
> -	     registers are not used in mem insn.  */
> -	  FOR_EACH_INSN_DEF (def, mii->inc_insn)
> -	    if (!reg_overlap_mentioned_p (DF_REF_REG (def), mii->mem_reg0))
> +	    else
>   	      {
> -		df_ref use;
> -		FOR_EACH_INSN_USE (use, mii->mem_insn)
> +		df_ref use, def2;
> +		FOR_EACH_INSN_USE (use, mii->inc_insn)
>   		  if (reg_overlap_mentioned_p (DF_REF_REG (def),
>   					       DF_REF_REG (use)))
>   		    {
>   		      if (sched_verbose >= 5)
>   			fprintf (sched_dump,
> -				 "inc clobber used in store failure.\n");
> +				 "mem def conflict with inc use failure.\n");
>   		      goto next;
>   		    }
> +		/* DEFS in inc_insn other than mem_reg0 should be always
> +		   clobbers.

It can be unused set too (not only clobbers) and single_set will still 
return non-null.

inc_insn: def R, def R2, unused R2
mem_insn: use R, def R2
...
use R2

But the code still works in this case.  So I'd only modify the comment.

  If both inc_insn and mem_insn clobber the same
> +		   register, it is fine, but avoid the case where mem_insn e.g.
> +		   sets CC and originally earlier inc_insn clobbers it.  */
> +		if ((DF_REF_FLAGS (def) & DF_REF_MUST_CLOBBER) == 0
> +		    && backwards)
> +		  FOR_EACH_INSN_DEF (def2, mii->inc_insn)
> +		    if (reg_overlap_mentioned_p (DF_REF_REG (def),
> +						 DF_REF_REG (def2)))
> +		      {
> +			if (sched_verbose >= 5)
> +			  fprintf (sched_dump,
> +				   "mem def conflict with inc def failure.\n");
> +			goto next;
> +		      }
>   	      }
>
> +	  /* The inc instruction could have clobbers, make sure those
> +	     registers are not used in mem insn, if mem_insn is originally
> +	     earlier than inc_insn.  */
> +	  if (!backwards)
> +	    FOR_EACH_INSN_DEF (def, mii->inc_insn)
> +	      if (!reg_overlap_mentioned_p (DF_REF_REG (def), mii->mem_reg0))
> +		{
> +		  df_ref use;
> +		  FOR_EACH_INSN_USE (use, mii->mem_insn)
> +		    if (reg_overlap_mentioned_p (DF_REF_REG (def),
> +						 DF_REF_REG (use)))
> +		      {
> +			if (sched_verbose >= 5)
> +			  fprintf (sched_dump,
> +				   "inc def conflict with mem use failure.\n");
> +			goto next;
> +		      }
> +		}
> +
>   	  newaddr = mii->inc_input;
>   	  if (mii->mem_index != NULL_RTX)
>   	    newaddr = gen_rtx_PLUS (GET_MODE (newaddr), newaddr,
>
>
> 	Jakub