This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 3/9] ifcvt: Only created temporaries as needed.


Robin Dapp <rdapp@linux.ibm.com> writes:
> noce_convert_multiple_sets creates temporaries for the destination of
> every emitted cmov and expects subsequent passes to get rid of them.  This
> does not happen every time and even if the temporaries are removed, code
> generation can be affected adversely.  In this patch, temporaries are
> only created if the destination of a set is used in an emitted condition
> check.

Still digesting the series, but a couple of implementation questions:

>
> ---
>  gcc/ifcvt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index e95ff9ee9b0..253b8a96c1a 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -99,6 +99,10 @@ 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_temps (basic_block bb,
> +                              hash_map<rtx, bool> *need_temp,
> +                              rtx cond);
> +
>  
>  /* Count the number of non-jump active insns in BB.  */
>  
> @@ -3145,6 +3149,12 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>    auto_vec<rtx_insn *> unmodified_insns;
>    int count = 0;
>  
> +  hash_map<rtx, bool> need_temps;
> +
> +  check_need_temps (then_bb, &need_temps, cond);

Is the separate need_temps scan required for correctness?  It looked
like we could test:

      if (reg_overlap_mentioned_p (dest, cond))
	...

on-the-fly during the main noce_convert_multiple_sets loop.

> +
> +  hash_map<rtx, bool> temps_created;
> +
>    FOR_BB_INSNS (then_bb, insn)
>      {
>        /* Skip over non-insns.  */
> @@ -3155,10 +3165,20 @@ 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 new_val = SET_SRC (set);
>        rtx old_val = target;
>  
> +      rtx dest = SET_DEST (set);
> +
> +      rtx temp;
> +      if (need_temps.get (dest))
> +       {
> +         temp = gen_reg_rtx (GET_MODE (target));
> +         temps_created.put (target, true);
> +       }
> +      else
> +       temp = 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.  */
> @@ -3242,8 +3262,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
>  
>    /* Now fixup the assignments.  */
>    for (int i = 0; i < count; i++)
> -    noce_emit_move_insn (targets[i], temporaries[i]);
> -
> +    if (temps_created.get(targets[i]) && targets[i] != temporaries[i])
> +      noce_emit_move_insn (targets[i], temporaries[i]);

Would it work to set temporaries[i] to targets[i] whenever a temporary
isn't needed, and avoid temps_created?

Thanks,
Richard

>  
>    /* Actually emit the sequence if it isn't too expensive.  */
>    rtx_insn *seq = get_insns ();
> @@ -3749,6 +3769,34 @@ check_cond_move_block (basic_block bb,
>    return TRUE;
>  }
>  
> +/* Check for which sets we need to emit temporaries to hold the destination of
> +   a conditional move.  */
> +static void
> +check_need_temps (basic_block bb, hash_map<rtx, bool> *need_temp, rtx cond)
> +{
> +  rtx_insn *insn;
> +
> +  FOR_BB_INSNS (bb, insn)
> +    {
> +      rtx set, dest;
> +
> +      if (!active_insn_p (insn))
> +	continue;
> +
> +      set = single_set (insn);
> +      if (set == NULL_RTX)
> +	continue;
> +
> +      dest = SET_DEST (set);
> +
> +      /* noce_emit_cmove will emit the condition check every time it is called
> +         so we need a temp register if the destination is modified.  */
> +      if (reg_overlap_mentioned_p (dest, cond))
> +	need_temp->put (dest, true);
> +    }
> +}
> +
> +
>  /* 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]