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

[Vineet Gupta <vineetg@rivosinc.com>]

On 10/19/23 23:50, Ajit Agarwal wrote:
> Hello All:
>
> This version 9 of the patch uses abi interfaces to remove zero and sign extension elimination.
> Bootstrapped and regtested on powerpc-linux-gnu.
>
> In this version (version 9) 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.

As has been trend in the past, I don't think all the review comments 
have been addressed.
The standard practice is to reply to reviewer's email and say yay/nay 
explicitly to each comment. Some of my comments in [1a] are still not 
resolved, importantly the last two. To be fair you did reply [1b] but 
the comments were not addressed explicitly.

[1a] https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630814.html
[1b] https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630865.html

Anyhow I gave this a try for RISC-V, specially after [2a][2b] as I was 
curious to see if this uncovers REE handling extraneous extensions which 
could potentially be eliminated in Expand itself, which is generally 
better as it happens earlier in the pipeline.

[2a] 2023-10-16 8eb9cdd14218 expr: don't clear SUBREG_PROMOTED_VAR_P 
flag for a promoted subreg [target/111466]
[2b] https://gcc.gnu.org/pipermail/gcc-patches/2023-October/631818.html

Bad news is with the patch, we fail to even bootstrap risc-v, buckles 
over when building libgcc itself.

The reproducer is embarrassingly simple, build with -O2:

float a;
b() { return a; }

See details below....

> Thanks & Regards
> Ajit
>
> ree: 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-10-20  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): New function.
>          (abi_target_promote_function_mode): New function.
>
> gcc/testsuite/ChangeLog:
>
>          * g++.target/powerpc/zext-elim-3.C
> ---
>   gcc/ree.cc                                    | 151 +++++++++++++++++-
>   .../g++.target/powerpc/zext-elim-3.C          |  13 ++
>   2 files changed, 161 insertions(+), 3 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..9f21f0e9907 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,124 @@ 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
> +   a return registers.  */
> +
> +static bool
> +abi_extension_candidate_return_reg_p (/*rtx_insn *insn, */int regno)
> +{
> +  if (targetm.calls.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);
> +  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 (/*insn,*/ REGNO (orig_src)))
> +    return false;
> +
> +  /* Mode of destination and source should be different.  */
> +  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);
> +
> +  /* REGNO of source and destination should be same if not
> +      promoted.  */
> +  if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src))
> +    return false;
> +
> +  return true;
> +}
> +
> +/* 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 (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 (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 +889,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);
>   
> @@ -1036,6 +1160,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;
> +

Is this early exit OK ? It skips a subsequent apply_change_group() call 
which could potentially fail and thus extension would be undone.
And even for the passing case, we do want those instructions to be 
merged normally. So I don't understand how this change is useful at all ?

FWIW for my test, w/o the patch: apply_change_group would fail (likely 
missing some combine pattern) and undo the merge. However w/ patch we 
just return/continue, keeping the merged but invalid insn which ICE in 
next pass cprop_hardreg as failure to recog that insn.

Some more details in case you are curious:

Coming into REE we have

(insn 7 6 13 2 (set (reg:SI 10 a0 [136])  #DEF
         (fix:SI (reg:SF 47 fa5 [orig:138 a ] [138])))
                  {fix_truncsfsi2}

(insn 13 7 14 2 (set (reg/i:DI 10 a0)    # extension
         (sign_extend:DI (reg:SI 10 a0 [136])))
                  {extendsidi2}

These are merged into unrecog insn as of now:

(insn 7 6 14 2 (set (reg:DI 10 a0)
         (sign_extend:DI (fix:SI (reg:SF 47 fa5 [orig:138 a ] [138]))))

Thx,
-Vineet

>     if (merge_successful)
>       {
>         /* Commit the changes here if possible
> @@ -1112,6 +1245,14 @@ 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);
> +
> +      if (!defs && abi_extension_candidate_argno_p (/*code,*/ REGNO (reg)))
> +	{
> +	  ext_cand e = {expr, code, mode, insn};
> +	  insn_list->safe_push (e);
> +	  return;
> +	}
>   
>         /* Zero-extension of an undefined value is partly defined (it's
>   	 completely undefined for sign-extension, though).  So if there exists
> @@ -1131,7 +1272,6 @@ add_removable_extension (const_rtx expr, rtx_insn *insn,
>   	}
>   
>         /* Second, make sure we can get all the reaching definitions.  */
> -      defs = get_defs (insn, reg, NULL);
>         if (!defs)
>   	{
>   	  if (dump_file)
> @@ -1321,7 +1461,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 +1486,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" } } */