Re: [PATCH 1/7] ifcvt: Check if cmovs are needed.

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

Robin Dapp <rdapp@linux.ibm.com> writes:
> Hi Richard,
>
> thanks for going through the patch set.
>
>> A hash_set might be simpler, given that the code only enters insns
>> for which the bool is false.  “rtx_insn *” would be better than rtx.
>
> Right, changed that.
>
>> Do you mean the sets might be removed or that the checks might be
>> removed?
>
> It's actually the checks that are removed.  I clarified and amended the 
> comments.
>
>> The patch is quite hard to review on its own, since nothing actually
>> uses this variable.  It's also not obvious how the
>> reg_overlap_mentioned_p code works if the old target is referenced
>> later.
>> 
>> Could you refactor the series a bit so that each patch is
>> self-contained? It's OK if that means fewer patches.
> The attached v2 makes use of need_cmov now, I hope this makes it a bit 
> clearer.  Regarding the reg_overlap_mentioned_p: it is used to detect 
> the swap idioms as well as when a cmov destination is used in the 
> condition as well.  If needed this could be two separate patches (most 
> of the second patch would be comments, though).

Thanks for the update.  No need for further splitting, this looks like a
nice self-contained patch as it is.

> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 017944f4f79..a5e55456d88 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -98,6 +98,7 @@ static int dead_or_predicable (basic_block, basic_block, basic_block,
>  			       edge, int);
>  static void noce_emit_move_insn (rtx, rtx);
>  static rtx_insn *block_has_only_trap (basic_block);
> +static void check_need_cmovs (basic_block, hash_set<rtx_insn *> *);
>  \f
>  /* Count the number of non-jump active insns in BB.  */
>  
> @@ -3203,6 +3204,10 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>    auto_vec<rtx_insn *> unmodified_insns;
>    int count = 0;
>  
> +  hash_set<rtx_insn *> need_no_cmov;
> +
> +  check_need_cmovs (then_bb, &need_no_cmov);
> +
>    FOR_BB_INSNS (then_bb, insn)
>      {
>        /* Skip over non-insns.  */
> @@ -3213,26 +3218,47 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>        gcc_checking_assert (set);
>  
>        rtx target = SET_DEST (set);
> -      rtx temp = gen_reg_rtx (GET_MODE (target));
> +      rtx temp;
>        rtx new_val = SET_SRC (set);
>        rtx old_val = target;
>  
> -      /* If we were supposed to read from an earlier write in this block,
> -	 we've changed the register allocation.  Rewire the read.  While
> -	 we are looking, also try to catch a swap idiom.  */
> -      for (int i = count - 1; i >= 0; --i)
> -	if (reg_overlap_mentioned_p (new_val, targets[i]))
> -	  {
> -	    /* Catch a "swap" style idiom.  */
> -	    if (find_reg_note (insn, REG_DEAD, new_val) != NULL_RTX)
> -	      /* The write to targets[i] is only live until the read
> -		 here.  As the condition codes match, we can propagate
> -		 the set to here.  */
> -	      new_val = SET_SRC (single_set (unmodified_insns[i]));
> -	    else
> -	      new_val = temporaries[i];
> -	    break;
> -	  }

Don't we still need this code (without the REG_DEAD handling) for the
case in which…

> +      /* As we are transforming
> +	 if (x > y)
> +	   {
> +	     a = b;
> +	     c = d;
> +	   }
> +	 into
> +	   a = (x > y) ...
> +	   c = (x > y) ...
> +
> +	 we potentially check x > y before every set.
> +	 Even though the check might be removed by subsequent passes, this means
> +	 that we cannot transform
> +	   if (x > y)
> +	     {
> +	       x = y;
> +	       ...
> +	     }
> +	 into
> +	   x = (x > y) ...
> +	   ...
> +	 since this would invalidate x and the following to-be-removed checks.
> +	 Therefore we introduce a temporary every time we are about to
> +	 overwrite a variable used in the check.  Costing of a sequence with
> +	 these is going to be inaccurate so only use temporaries when
> +	 needed.  */
> +      if (reg_overlap_mentioned_p (target, cond))
> +	temp = gen_reg_rtx (GET_MODE (target));

…this code triggers?  I don't see otherwise how later uses of x would
pick up “temp” instead of the original target.  E.g. suppose we had:

    if (x > y)
      {
        x = …;
        z = x; // x does not die here
      }

Without the loop, it looks like z would pick up the old value of x
(used in the comparison) instead of the new one.

> +      else
> +	temp = target;
> +
> +      /* We have identified swap-style idioms in check_need_cmovs.  A normal
> +	 set will need to be a cmov while the first instruction of a swap-style
> +	 idiom can be a regular move.  This helps with costing.  */
> +      bool need_cmov = true;
> +      if (need_no_cmov.contains (insn))
> +	need_cmov = false;

Would be simpler as:

      bool need_cmov = !need_no_cmov.contains (insn);
>  
>        /* If we had a non-canonical conditional jump (i.e. one where
>  	 the fallthrough is to the "else" case) we need to reverse
> @@ -3275,9 +3301,22 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>  	  old_val = lowpart_subreg (dst_mode, old_val, src_mode);
>  	}
>  
> -      /* Actually emit the conditional move.  */
> -      rtx temp_dest = noce_emit_cmove (if_info, temp, cond_code,
> -				       x, y, new_val, old_val);
> +      rtx temp_dest = NULL_RTX;
> +
> +      if (need_cmov)
> +	{
> +	  /* Actually emit the conditional move.  */
> +	  temp_dest = noce_emit_cmove (if_info, temp, cond_code,
> +					   x, y, new_val, old_val);
> +	}
> +      else
> +	{
> +	  if (if_info->then_else_reverse)
> +	    noce_emit_move_insn (temp, old_val);
> +	  else
> +	    noce_emit_move_insn (temp, new_val);
> +	  temp_dest = temp;
> +	}
>  
>        /* If we failed to expand the conditional move, drop out and don't
>  	 try to continue.  */

I think this comment and the associated null check belong in the
“if (need_cmov)”

> @@ -3808,6 +3847,65 @@ check_cond_move_block (basic_block bb,
>    return TRUE;
>  }
>  
> +/* Find local swap-style idioms in BB and mark the first insn (1)
> +   that is only a temporary as not needing a conditional move as
> +   it is going to be dead afterwards anyway.
> +
> +     (1) int tmp = a;
> +	 a = b;
> +	 b = tmp;
> +
> +	 ifcvt
> +	 -->
> +
> +	 load tmp,a
> +	 cmov a,b
> +	 cmov b,tmp   */
> +
> +static void
> +check_need_cmovs (basic_block bb, hash_set<rtx_insn *> *need_no_cmov)
> +{
> +  rtx_insn *insn;
> +  int count = 0;
> +  auto_vec<rtx_insn *> insns;
> +  auto_vec<rtx> dests;
> +
> +  /* Iterate over all SETs, storing the destinations
> +     in DEST.  If we hit a SET that reads from a destination
> +     that we have seen before and the corresponding register
> +     is dead afterwards, it must be a swap.  */

This is probably pedantic, but I guess it could also be a missed
forward-propagation opportunity.  E.g. there's no reason in principle
why we couldn't see:

  int tmp = a;
  b = tmp; // tmp dies here

The code handles that case correctly, but it isn't a swap.

> +  FOR_BB_INSNS (bb, insn)
> +    {
> +      rtx set, src, dest;
> +
> +      if (!active_insn_p (insn))
> +	continue;
> +
> +      set = single_set (insn);
> +      if (set == NULL_RTX)
> +	continue;
> +
> +      src = SET_SRC (set);
> +      dest = SET_DEST (set);
> +
> +      /* Check if the current SET's source is the same
> +	 as any previously seen destination.
> +	 This is quadratic but the number of insns in BB
> +	 is bounded by PARAM_MAX_RTL_IF_CONVERSION_INSNS.  */
> +      for (int i = count - 1; i >= 0; --i)
> +	{
> +	  if (reg_overlap_mentioned_p (src, dests[i])
> +	      && find_reg_note (insn, REG_DEAD, src) != NULL_RTX)
> +	    need_no_cmov->add (insns[i]);
> +	}

Minor formatting nit, sorry, but the braces aren't needed.

This is really a comment about the existing swap recognition code,
but I think the reg_overlap_mentioned_p check would be more obviously
correct if we guard the “for” loop with:

      if (REG_P (src))

This guarantees that any previous instructions are equal to SRC or
subregs of it.  I guess it might be more efficient too.

Thanks,
Richard