Re: [PATCH V14 4/4] ree: Improve ree pass using defined abi interfaces

[Vineet Gupta <vineetg@rivosinc.com>]

On 10/24/23 10:03, Ajit Agarwal wrote:
> Hello Vineet, Jeff and Bernhard:
>
> This version 14 of the patch uses abi interfaces to remove zero and sign extension elimination.
> This fixes aarch64 regressions failures with aggressive CSE.

Once again, this information belong between the two "---" lines that you 
added for v6 and stopped updating.

And it seems the only code difference between v13 and v14 is

-  return tgt_mode == mode;
+  if (tgt_mode == mode)
+    return true;
+  else
+    return false;

How does that make any difference ?

-Vineet

>
> Bootstrapped and regtested on powerpc-linux-gnu.
>
> In this version (version 14) of the patch following review comments are incorporated.
>
> a) Removal of hard code zero_extend and sign_extend  in abi interfaces.
> b) Source and destination with different registers are considered.
> c) Further enhancements.
> d) Added sign extension elimination using abi interfaces.
> d) Addressed remaining review comments from Vineet.
> e) Addressed review comments from Bernhard.
> f) Fix aarch64 regressions failure.
>
> Please let me know if there is anything missing in this patch.
>
> Ok for trunk?
>
> Thanks & Regards
> Ajit
>
> ree: Improve ree pass using defined abi interfaces
>
> For rs6000 target we see zero and sign extend with missing
> definitions. Improved to eliminate such zero and sign extension
> using defined ABI interfaces.
>
> 2023-10-24  Ajit Kumar Agarwal  <aagarwa1@linux.ibm.com>
>
> gcc/ChangeLog:
>
>          * ree.cc (combine_reaching_defs): Eliminate zero_extend and sign_extend
>          using 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): New function.
>          (abi_target_promote_function_mode): New function.
>
> gcc/testsuite/ChangeLog:
>
>          * g++.target/powerpc/zext-elim-3.C
> ---
> changes since v6:
>    - Added missing abi interfaces.
>    - Rearranging and restructuring the code.
>    - Removal of hard coded zero extend and sign extend in abi interfaces.
>    - Relaxed different registers with source and destination in abi interfaces.
>    - Using CSE in abi interfaces.
>    - Fix aarch64 regressions.
>    - Add Sign extension removal in abi interfaces.
>    - Modified comments as per coding convention.
>    - Modified code as per coding convention.
>    - Fix bug bootstrapping RISCV failures.
> ---
>   gcc/ree.cc                                    | 147 +++++++++++++++++-
>   .../g++.target/powerpc/zext-elim-3.C          |  13 ++
>   2 files changed, 154 insertions(+), 6 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..f557b49b366 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, false otherwise.  */
> +
> +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 regno is a return register.  */
> +
> +static inline bool
> +abi_extension_candidate_return_reg_p (int regno)
> +{
> +  if (targetm.calls.function_value_regno_p (regno))
> +    return true;
> +
> +  return false;
> +}
> +
> +/* Return TRUE if the following conditions are satisfied.
> +
> +  a) reg source operand is argument register and not return register.
> +  b) mode of source and destination operand are different.
> +  c) if not promoted REGNO of source and destination operand are same.  */
> +
> +static bool
> +abi_extension_candidate_p (rtx_insn *insn)
> +{
> +  rtx set = single_set (insn);
> +  machine_mode dst_mode = GET_MODE (SET_DEST (set));
> +  rtx orig_src = XEXP (SET_SRC (set), 0);
> +
> +  if (!FUNCTION_ARG_REGNO_P (REGNO (orig_src))
> +      || abi_extension_candidate_return_reg_p (REGNO (orig_src)))
> +    return false;
> +
> +  /* Return FALSE if mode of destination and source is same.  */
> +  if (dst_mode == GET_MODE (orig_src))
> +    return false;
> +
> +  machine_mode mode = GET_MODE (XEXP (SET_SRC (set), 0));
> +  bool promote_p = abi_target_promote_function_mode (mode);
> +
> +  /* Return FALSE if promote is false and REGNO of source and destination
> +     is different.  */
> +  if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src))
> +    return false;
> +
> +  return true;
> +}
> +
> +/* Return TRUE if regno is an argument register.  */
> +
> +static inline bool
> +abi_extension_candidate_argno_p (int regno)
> +{
> +  return FUNCTION_ARG_REGNO_P (regno);
> +}
> +
> +/* 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 (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;
> +	}
> +     }
> +
> +  rtx set = single_set (insn);
> +
> +  if (GET_CODE (SET_SRC (set)) == SIGN_EXTEND)
> +    {
> +      machine_mode mode = GET_MODE (XEXP (SET_SRC (set), 0));
> +      bool promote_p = abi_target_promote_function_mode (mode);
> +
> +      return promote_p;
> +    }
> +  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 (cand->insn);
>   
>     outcome = make_defs_and_copies_lists (cand->insn, set_pat, state);
>   
> @@ -1116,9 +1236,12 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
>         /* 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)))
> +	 uninitialized and not argument register, 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))
> +	  && !abi_extension_candidate_argno_p (REGNO (reg)))
>   	{
>   	  if (dump_file)
>   	    {
> @@ -1130,10 +1253,17 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
>   	  return;
>   	}
>   
> -      /* Second, make sure we can get all the reaching definitions.  */
> +      /* Second, make sure we can get all the reaching definitions or reg is
> +	 argument register.  */
>         defs = get_defs (insn, reg, NULL);
>         if (!defs)
>   	{
> +	  if (abi_extension_candidate_argno_p (REGNO (reg)))
> +	    {
> +	      ext_cand e = {expr, code, mode, insn};
> +	      insn_list->safe_push (e);
> +	      return;
> +	    }
>   	  if (dump_file)
>   	    {
>   	      fprintf (dump_file, "Cannot eliminate extension:\n");
> @@ -1321,7 +1451,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;
> @@ -1345,6 +1476,10 @@ find_and_remove_re (void)
>     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 ())
> +	continue;
> +
>         rtx_insn *def_insn = reinsn_copy_list[i + 1];
>   
>         /* Use the mode of the destination of the defining 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" } } */