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

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

Sorry for the slow review.

Robin Dapp <rdapp@linux.ibm.com> writes:
> When if-converting multiple SETs and we encounter a swap-style idiom
>
>   if (a > b)
>     {
>       tmp = c;   // [1]
>       c = d;
>       d = tmp;
>     }
>
> ifcvt should not generate a conditional move for the instruction at
> [1].
>
> In order to achieve that, this patch goes through all relevant SETs
> and marks the relevant instructions.  This helps to evaluate costs.
>
> On top, only generate temporaries if the current cmov is going to
> overwrite one of the comparands of the initial compare.
> ---
>  gcc/ifcvt.c | 104 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 87 insertions(+), 17 deletions(-)
>
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 017944f4f79..eef6490626a 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_map<rtx, bool> *);
>  \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_map<rtx, bool> need_cmovs;

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.

> +
> +  check_need_cmovs (then_bb, &need_cmovs);
> +
>    FOR_BB_INSNS (then_bb, insn)
>      {
>        /* Skip over non-insns.  */
> @@ -3213,26 +3218,38 @@ 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;
> -	  }
> +      /* As we are transforming
> +	 if (x > y)
> +	   a = b;
> +	   c = d;

Looks like some missing braces here.

> +	 into
> +	   a = (x > y) ...
> +	   c = (x > y) ...
> +
> +	 we potentially check x > y before every set here.
> +	 (Even though might be removed by subsequent passes.)

Do you mean the sets might be removed or that the checks might be removed?

> +	 We cannot transform
> +	   if (x > y)
> +	     x = y;
> +	     ...
> +	 into
> +	   x = (x > y) ...
> +	   ...
> +	 since this would invalidate x.  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.  */
> +      if (reg_overlap_mentioned_p (target, cond))
> +	temp = gen_reg_rtx (GET_MODE (target));
> +      else
> +	temp = target;
> +
> +      bool need_cmov = true;
> +      if (need_cmovs.get (insn))
> +	need_cmov = false;

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.

Thanks,
Richard

>        /* If we had a non-canonical conditional jump (i.e. one where
>  	 the fallthrough is to the "else" case) we need to reverse
> @@ -3808,6 +3825,59 @@ 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_map<rtx, bool> *need_cmov)
> +{
> +  rtx_insn *insn;
> +  int count = 0;
> +  auto_vec<rtx> insns;
> +  auto_vec<rtx> dests;
> +
> +  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);
> +
> +      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_cmov->put (insns[i], false);
> +	    }
> +	}
> +
> +      insns.safe_push (insn);
> +      dests.safe_push (dest);
> +
> +      count++;
> +    }
> +}
> +
>  /* Given a basic block BB suitable for conditional move conversion,
>     a condition COND, and pointer maps THEN_VALS and ELSE_VALS containing
>     the register values depending on COND, emit the insns in the block as