Re: [PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets

[Richard Sandiford <richard.sandiford@arm.com>]

Sorry for the slow review.

Manolis Tsamis <manolis.tsamis@vrull.eu> writes:
> This is an extension of what was done in PR106590.
>
> Currently if a sequence generated in noce_convert_multiple_sets clobbers the
> condition rtx (cc_cmp or rev_cc_cmp) then only seq1 is used afterwards
> (sequences that emit the comparison itself). Since this applies only from the
> next iteration it assumes that the sequences generated (in particular seq2)
> doesn't clobber the condition rtx itself before using it in the if_then_else,
> which is only true in specific cases (currently only register/subregister moves
> are allowed).
>
> This patch changes this so it also tests if seq2 clobbers cc_cmp/rev_cc_cmp in
> the current iteration. This makes it possible to include arithmetic operations
> in noce_convert_multiple_sets.
>
> It also makes the code that checks whether the condition is used outside of the
> if_then_else emitted more robust.
>
> gcc/ChangeLog:
>
> 	* ifcvt.cc (check_for_cc_cmp_clobbers): Use modified_in_p instead.
> 	(noce_convert_multiple_sets_1): Don't use seq2 if it clobbers cc_cmp.
> 	Refactor the code that sets read_comparison.
>
> Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
> ---
>
> (no changes since v1)
>
>  gcc/ifcvt.cc | 106 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 59 insertions(+), 47 deletions(-)
>
> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index 58ed42673e5..763a25f816e 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -3592,20 +3592,6 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>    return true;
>  }
>  
> -/* Helper function for noce_convert_multiple_sets_1.  If store to
> -   DEST can affect P[0] or P[1], clear P[0].  Called via note_stores.  */
> -
> -static void
> -check_for_cc_cmp_clobbers (rtx dest, const_rtx, void *p0)
> -{
> -  rtx *p = (rtx *) p0;
> -  if (p[0] == NULL_RTX)
> -    return;
> -  if (reg_overlap_mentioned_p (dest, p[0])
> -      || (p[1] && reg_overlap_mentioned_p (dest, p[1])))
> -    p[0] = NULL_RTX;
> -}
> -
>  /* This goes through all relevant insns of IF_INFO->then_bb and tries to
>     create conditional moves.  In case a simple move sufficis the insn
>     should be listed in NEED_NO_CMOV.  The rewired-src cases should be
> @@ -3731,36 +3717,67 @@ noce_convert_multiple_sets_1 (struct noce_if_info *if_info,
>  	 creating an additional compare for each.  If successful, costing
>  	 is easier and this sequence is usually preferred.  */
>        if (cc_cmp)
> -	seq2 = try_emit_cmove_seq (if_info, temp, cond,
> -				   new_val, old_val, need_cmov,
> -				   &cost2, &temp_dest2, cc_cmp, rev_cc_cmp);
> +	{
> +	  seq2 = try_emit_cmove_seq (if_info, temp, cond,
> +				     new_val, old_val, need_cmov,
> +				     &cost2, &temp_dest2, cc_cmp, rev_cc_cmp);
> +
> +	  /* The if_then_else in SEQ2 may be affected when cc_cmp/rev_cc_cmp is
> +	     clobbered.  We can't safely use the sequence in this case.  */
> +	  if (seq2 && (modified_in_p (cc_cmp, seq2)
> +	      || (rev_cc_cmp && modified_in_p (rev_cc_cmp, seq2))))
> +	    seq2 = NULL;

It looks like this still has the problem that I mentioned in the
previous round: that modified_in_p only checks the first instruction
in seq2, not the whole sequence.  Or is that the intention?

Thanks,
Richard

> +	}
>  
>        /* The backend might have created a sequence that uses the
> -	 condition.  Check this.  */
> +	 condition as a value.  Check this.  */
> +
> +      /* We cannot handle anything more complex than a reg or constant.  */
> +      if (!REG_P (XEXP (cond, 0)) && !CONSTANT_P (XEXP (cond, 0)))
> +	read_comparison = true;
> +
> +      if (!REG_P (XEXP (cond, 1)) && !CONSTANT_P (XEXP (cond, 1)))
> +	read_comparison = true;
> +
>        rtx_insn *walk = seq2;
> -      while (walk)
> +      int if_then_else_count = 0;
> +      while (walk && !read_comparison)
>  	{
> -	  rtx set = single_set (walk);
> +	  rtx exprs_to_check[2];
> +	  unsigned int exprs_count = 0;
>  
> -	  if (!set || !SET_SRC (set))
> +	  rtx set = single_set (walk);
> +	  if (set && XEXP (set, 1)
> +	      && GET_CODE (XEXP (set, 1)) == IF_THEN_ELSE)
>  	    {
> -	      walk = NEXT_INSN (walk);
> -	      continue;
> +	      /* We assume that this is the cmove created by the backend that
> +		 naturally uses the condition.  */
> +	      exprs_to_check[exprs_count++] = XEXP (XEXP (set, 1), 1);
> +	      exprs_to_check[exprs_count++] = XEXP (XEXP (set, 1), 2);
> +	      if_then_else_count++;
>  	    }
> +	  else if (NONDEBUG_INSN_P (walk))
> +	    exprs_to_check[exprs_count++] = PATTERN (walk);
>  
> -	  rtx src = SET_SRC (set);
> +	  /* Bail if we get more than one if_then_else because the assumption
> +	     above may be incorrect.  */
> +	  if (if_then_else_count > 1)
> +	    {
> +	      read_comparison = true;
> +	      break;
> +	    }
>  
> -	  if (XEXP (set, 1) && GET_CODE (XEXP (set, 1)) == IF_THEN_ELSE)
> -	    ; /* We assume that this is the cmove created by the backend that
> -		 naturally uses the condition.  Therefore we ignore it.  */
> -	  else
> +	  for (unsigned int i = 0; i < exprs_count; i++)
>  	    {
> -	      if (reg_mentioned_p (XEXP (cond, 0), src)
> -		  || reg_mentioned_p (XEXP (cond, 1), src))
> -		{
> -		  read_comparison = true;
> -		  break;
> -		}
> +	      subrtx_iterator::array_type array;
> +	      FOR_EACH_SUBRTX (iter, array, exprs_to_check[i], NONCONST)
> +		if (*iter != NULL_RTX
> +		    && (reg_overlap_mentioned_p (XEXP (cond, 0), *iter)
> +		    || reg_overlap_mentioned_p (XEXP (cond, 1), *iter)))
> +		  {
> +		    read_comparison = true;
> +		    break;
> +		  }
>  	    }
>  
>  	  walk = NEXT_INSN (walk);
> @@ -3788,21 +3805,16 @@ noce_convert_multiple_sets_1 (struct noce_if_info *if_info,
>  	  return false;
>  	}
>  
> -      if (cc_cmp)
> +      if (cc_cmp && seq == seq1)
>  	{
> -	  /* Check if SEQ can clobber registers mentioned in
> -	     cc_cmp and/or rev_cc_cmp.  If yes, we need to use
> -	     only seq1 from that point on.  */
> -	  rtx cc_cmp_pair[2] = { cc_cmp, rev_cc_cmp };
> -	  for (walk = seq; walk; walk = NEXT_INSN (walk))
> +	  /* Check if SEQ can clobber registers mentioned in cc_cmp/rev_cc_cmp.
> +	     If yes, we need to use only seq1 from that point on.
> +	     Only check when we use seq1 since we have already tested seq2.  */
> +	  if (modified_in_p (cc_cmp, seq)
> +	      || (rev_cc_cmp && modified_in_p (rev_cc_cmp, seq)))
>  	    {
> -	      note_stores (walk, check_for_cc_cmp_clobbers, cc_cmp_pair);
> -	      if (cc_cmp_pair[0] == NULL_RTX)
> -		{
> -		  cc_cmp = NULL_RTX;
> -		  rev_cc_cmp = NULL_RTX;
> -		  break;
> -		}
> +	      cc_cmp = NULL_RTX;
> +	      rev_cc_cmp = NULL_RTX;
>  	    }
>  	}