Re: PATCH v5 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces.

[Ajit Agarwal <aagarwa1@linux.ibm.com>]

All:

Ok for trunk. Please review.

Thanks & Regards
Ajit

On 01/06/23 10:53 am, Ajit Agarwal via Gcc-patches wrote:
> Hello All:
> 
> This new version of patch 4 use improve ree pass for rs6000 target using defined ABI interfaces.
> Bootstrapped and regtested on power64-linux-gnu.
> 
> Review comments incorporated.
> 
> Thanks & Regards
> Ajit
> 
> Improve ree pass for rs6000 target using defined abi interfaces
> 
> For rs6000 target we see redundant zero and sign
> extension and done to improve ree pass to eliminate
> such redundant zero and sign extension using defined
> ABI interfaces.
> 
> 2023-06-01  Ajit Kumar Agarwal  <aagarwa1@linux.ibm.com>
> 
> gcc/ChangeLog:
> 
> 	* ree.cc (combine_reaching_defs): Use of  zero_extend and sign_extend
> 	defined abi interfaces.
> 	(add_removable_extension): Use of defined abi interfaces for no
> 	reaching defs.
> 	(abi_extension_candidate_return_reg_p): New function.
> 	(abi_extension_candidate_p): New function.
> 	(abi_extension_candidate_argno_p): New function.
> 	(abi_handle_regs_without_defs_p): New function.
> 	(abi_target_promote_function_mode): New function.
> 
> gcc/testsuite/ChangeLog:
> 
>         * g++.target/powerpc/zext-elim-3.C
> ---
>  gcc/ree.cc                                    | 199 +++++++++++++++---
>  .../g++.target/powerpc/zext-elim-3.C          |  13 ++
>  2 files changed, 183 insertions(+), 29 deletions(-)
>  create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim-3.C
> 
> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index fc04249fa84..2025a7c43da 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -514,7 +514,8 @@ get_uses (rtx_insn *insn, rtx reg)
>      if (REGNO (DF_REF_REG (def)) == REGNO (reg))
>        break;
>  
> -  gcc_assert (def != NULL);
> +  if (def == NULL)
> +    return NULL;
>  
>    ref_chain = DF_REF_CHAIN (def);
>  
> @@ -750,6 +751,120 @@ get_extended_src_reg (rtx src)
>    return src;
>  }
>  
> +/* Return TRUE if target mode is equal to source mode of zero_extend
> +   or sign_extend otherwise false.  */
> +
> +static bool
> +abi_target_promote_function_mode (machine_mode mode)
> +{
> +  int unsignedp;
> +  machine_mode tgt_mode =
> +    targetm.calls.promote_function_mode (NULL_TREE, mode, &unsignedp,
> +					 NULL_TREE, 1);
> +
> +  if (tgt_mode == mode)
> +    return true;
> +  else
> +    return false;
> +}
> +
> +/* Return TRUE if the candidate insn is zero extend and regno is
> +   an return  registers.  */
> +
> +static bool
> +abi_extension_candidate_return_reg_p (rtx_insn *insn, int regno)
> +{
> +  rtx set = single_set (insn);
> +
> +  if (GET_CODE (SET_SRC (set)) !=  ZERO_EXTEND)
> +    return false;
> +
> +  if (FUNCTION_VALUE_REGNO_P (regno))
> +    return true;
> +
> +  return false;
> +}
> +
> +/* Return TRUE if reg source operand of zero_extend is argument registers
> +   and not return registers and source and destination operand are same
> +   and mode of source and destination operand are not same.  */
> +
> +static bool
> +abi_extension_candidate_p (rtx_insn *insn)
> +{
> +  rtx set = single_set (insn);
> +
> +  if (GET_CODE (SET_SRC (set)) !=  ZERO_EXTEND)
> +    return false;
> +
> +  machine_mode ext_dst_mode = GET_MODE (SET_DEST (set));
> +  rtx orig_src = XEXP (SET_SRC (set),0);
> +
> +  bool copy_needed
> +    = (REGNO (SET_DEST (set)) != REGNO (XEXP (SET_SRC (set), 0)));
> +
> +  if (!copy_needed && ext_dst_mode != GET_MODE (orig_src)
> +      && FUNCTION_ARG_REGNO_P (REGNO (orig_src))
> +      && !abi_extension_candidate_return_reg_p (insn, REGNO (orig_src)))
> +    return true;
> +
> +  return false;
> +}
> +
> +/* Return TRUE if the candidate insn is zero extend and regno is
> +   an argument registers.  */
> +
> +static bool
> +abi_extension_candidate_argno_p (rtx_code code, int regno)
> +{
> +  if (code !=  ZERO_EXTEND)
> +    return false;
> +
> +  if (FUNCTION_ARG_REGNO_P (regno))
> +    return true;
> +
> +  return false;
> +}
> +
> +/* Return TRUE if the candidate insn doesn't have defs and have
> + * uses without RTX_BIN_ARITH/RTX_COMM_ARITH/RTX_UNARY rtx class.  */
> +
> +static bool
> +abi_handle_regs_without_defs_p (rtx_insn *insn)
> +{
> +  if (side_effects_p (PATTERN (insn)))
> +    return false;
> +
> +  struct df_link *uses
> +    = get_uses (insn, SET_DEST (PATTERN (insn)));
> +
> +  if (!uses)
> +    return false;
> +
> +  for (df_link *use = uses; use; use = use->next)
> +    {
> +      if (!use->ref)
> +	return false;
> +
> +      if (BLOCK_FOR_INSN (insn)
> +	  != BLOCK_FOR_INSN (DF_REF_INSN (use->ref)))
> +	return false;
> +
> +      rtx_insn *use_insn = DF_REF_INSN (use->ref);
> +
> +      if (GET_CODE (PATTERN (use_insn)) == SET)
> +	{
> +	  rtx_code code = GET_CODE (SET_SRC (PATTERN (use_insn)));
> +
> +	  if (GET_RTX_CLASS (code) == RTX_BIN_ARITH
> +	      || GET_RTX_CLASS (code) == RTX_COMM_ARITH
> +	      || GET_RTX_CLASS (code) == RTX_UNARY)
> +	    return false;
> +	}
> +     }
> +  return true;
> +}
> +
>  /* This function goes through all reaching defs of the source
>     of the candidate for elimination (CAND) and tries to combine
>     the extension with the definition instruction.  The changes
> @@ -770,6 +885,11 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>  
>    state->defs_list.truncate (0);
>    state->copies_list.truncate (0);
> +  rtx orig_src = XEXP (SET_SRC (cand->expr),0);
> +
> +  if (abi_extension_candidate_p (cand->insn)
> +      && (!get_defs (cand->insn, orig_src, NULL)))
> +    return abi_handle_regs_without_defs_p (cand->insn);
>  
>    outcome = make_defs_and_copies_lists (cand->insn, set_pat, state);
>  
> @@ -1036,6 +1156,15 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>          }
>      }
>  
> +  rtx insn_set = single_set (cand->insn);
> +
> +  machine_mode mode = (GET_MODE (XEXP (SET_SRC (insn_set), 0)));
> +
> +  bool promote_p = abi_target_promote_function_mode (mode);
> +
> +  if (promote_p)
> +    return true;
> +
>    if (merge_successful)
>      {
>        /* Commit the changes here if possible
> @@ -1112,26 +1241,34 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
>        rtx reg = XEXP (src, 0);
>        struct df_link *defs, *def;
>        ext_cand *cand;
> +      defs = get_defs (insn, reg, NULL);
>  
>        /* Zero-extension of an undefined value is partly defined (it's
>  	 completely undefined for sign-extension, though).  So if there exists
>  	 a path from the entry to this zero-extension that leaves this register
>  	 uninitialized, removing the extension could change the behavior of
>  	 correct programs.  So first, check it is not the case.  */
> -      if (code == ZERO_EXTEND && !bitmap_bit_p (init_regs, REGNO (reg)))
> +      if (!defs && abi_extension_candidate_argno_p (code, REGNO (reg)))
>  	{
> -	  if (dump_file)
> -	    {
> -	      fprintf (dump_file, "Cannot eliminate extension:\n");
> -	      print_rtl_single (dump_file, insn);
> -	      fprintf (dump_file, " because it can operate on uninitialized"
> -			          " data\n");
> -	    }
> +	  ext_cand e = {expr, code, mode, insn};
> +	  insn_list->safe_push (e);
>  	  return;
>  	}
>  
> +       if ((code == ZERO_EXTEND
> +	    && !bitmap_bit_p (init_regs, REGNO (reg))))
> +	  {
> +	    if (dump_file)
> +	      {
> +		fprintf (dump_file, "Cannot eliminate extension:\n");
> +		print_rtl_single (dump_file, insn);
> +		fprintf (dump_file, " because it can operate on uninitialized"
> +				    " data\n");
> +	      }
> +	    return;
> +	  }
> +
>        /* Second, make sure we can get all the reaching definitions.  */
> -      defs = get_defs (insn, reg, NULL);
>        if (!defs)
>  	{
>  	  if (dump_file)
> @@ -1321,7 +1458,8 @@ find_and_remove_re (void)
>  	      && (REGNO (SET_DEST (set)) != REGNO (XEXP (SET_SRC (set), 0))))
>  	    {
>                reinsn_copy_list.safe_push (curr_cand->insn);
> -              reinsn_copy_list.safe_push (state.defs_list[0]);
> +	      if (state.defs_list.length() != 0)
> +		reinsn_copy_list.safe_push (state.defs_list[0]);
>  	    }
>  	  reinsn_del_list.safe_push (curr_cand->insn);
>  	  state.modified[INSN_UID (curr_cand->insn)].deleted = 1;
> @@ -1342,24 +1480,27 @@ find_and_remove_re (void)
>       Remember that the memory reference will be changed to refer to the
>       destination of the extention.  So we're actually emitting a copy
>       from the new destination to the old destination.  */
> -  for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
> -    {
> -      rtx_insn *curr_insn = reinsn_copy_list[i];
> -      rtx_insn *def_insn = reinsn_copy_list[i + 1];
> -
> -      /* Use the mode of the destination of the defining insn
> -	 for the mode of the copy.  This is necessary if the
> -	 defining insn was used to eliminate a second extension
> -	 that was wider than the first.  */
> -      rtx sub_rtx = *get_sub_rtx (def_insn);
> -      rtx set = single_set (curr_insn);
> -      rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
> -				 REGNO (XEXP (SET_SRC (set), 0)));
> -      rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
> -				 REGNO (SET_DEST (set)));
> -      rtx new_set = gen_rtx_SET (new_dst, new_src);
> -      emit_insn_after (new_set, def_insn);
> -    }
> +   for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
> +     {
> +       rtx_insn *curr_insn = reinsn_copy_list[i];
> +
> +       if ((i+1) < reinsn_copy_list.length())
> +	 {
> +	   rtx_insn *def_insn = reinsn_copy_list[i + 1];
> +	   /* Use the mode of the destination of the defining insn
> +	      for the mode of the copy.  This is necessary if the
> +	      defining insn was used to eliminate a second extension
> +	      that was wider than the first.  */
> +	   rtx sub_rtx = *get_sub_rtx (def_insn);
> +	   rtx set = single_set (curr_insn);
> +	   rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
> +				      REGNO (XEXP (SET_SRC (set), 0)));
> +	   rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
> +				      REGNO (SET_DEST (set)));
> +	   rtx new_set = gen_rtx_SET (new_dst, new_src);
> +	   emit_insn_after (new_set, def_insn);
> +	}
> +      }
>  
>    /* Delete all useless extensions here in one sweep.  */
>    FOR_EACH_VEC_ELT (reinsn_del_list, i, curr_insn)
> diff --git a/gcc/testsuite/g++.target/powerpc/zext-elim-3.C b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
> new file mode 100644
> index 00000000000..5a050df06ff
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/zext-elim-3.C
> @@ -0,0 +1,13 @@
> +/* { dg-options "-mcpu=power9 -O2" } */
> +
> +void *memset(void *b, int c, unsigned long len)
> +{
> +  unsigned long i;
> +
> +  for (i = 0; i < len; i++)
> +    ((unsigned char *)b)[i] = c;
> +
> +   return b;
> +}
> +
> +/* { dg-final { scan-assembler-not "\mrlwinm\M" } } */