Re: [PATCH v2] rtl-optimization: ppc backend generates unnecessary extension.

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

On Thu, 30 Mar 2023 17:30:43 +0530
Ajit Agarwal via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> Hello All:

Just some nits.

And this seems to be an incremental diff ontop of your v2.
You might want check for coding-style errors by running:
 contrib/check_GNU_style.py /tmp/ree-v2bis.patch

> 
> This patch added enhancement to extimination elimination for rs6000 target.

I think REE means redundant extension elimination.

> gcc/ChangeLog:
> 
> 	* ree.cc(is_feasible_elim_across_basic_blocks):

We often use the lispy _p suffix for predicates.
Maybe eliminate_across_bbs_p would be shorter.

> 	Add checks to enable extension elimination..
> 	* ree.cc(combine_reaching_defs): Add checks to enable
> 	extension elimination.
> ---
>  gcc/ree.cc | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 116 insertions(+), 5 deletions(-)
> 
> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index d05d37f9a23..7e4cce5cee4 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -253,6 +253,56 @@ struct ext_cand
>  
>  static int max_insn_uid;
>  
> +/* Get all the reaching definitions of an instruction.  The definitions are
> +   desired for REG used in INSN.  Return the definition list or NULL if a
> +   definition is missing.  If DEST is non-NULL, additionally push the INSN
> +   of the definitions onto DEST.  */
> +
> +static struct df_link *
> +get_defs (rtx_insn *insn, rtx reg, vec<rtx_insn *> *dest)
> +{
> +  df_ref use;
> +  struct df_link *ref_chain, *ref_link;
> +
> +  FOR_EACH_INSN_USE (use, insn)
> +    {
> +      if (GET_CODE (DF_REF_REG (use)) == SUBREG)
> +	return NULL;
> +      if (REGNO (DF_REF_REG (use)) == REGNO (reg))
> +	break;
> +    }
> +
> +  if (use == NULL) return NULL;

Missing newline before return.

> +
> +  gcc_assert (use != NULL);

Either the assert or the if but both is a bit much.

> +
> +  ref_chain = DF_REF_CHAIN (use);
> +
> +  for (ref_link = ref_chain; ref_link; ref_link = ref_link->next)
> +    {
> +      /* Problem getting some definition for this instruction.  */
> +      if (ref_link->ref == NULL)
> +	return NULL;
> +      if (DF_REF_INSN_INFO (ref_link->ref) == NULL)
> +	return NULL;
> +      /* As global regs are assumed to be defined at each function call
> +	 dataflow can report a call_insn as being a definition of REG.
> +	 But we can't do anything with that in this pass so proceed only
> +	 if the instruction really sets REG in a way that can be deduced
> +	 from the RTL structure.  */
> +      if (global_regs[REGNO (reg)]
> +	  && !set_of (reg, DF_REF_INSN (ref_link->ref)))
> +	return NULL;
> +    }
> +
> +  if (dest)
> +    for (ref_link = ref_chain; ref_link; ref_link = ref_link->next)
> +      dest->safe_push (DF_REF_INSN (ref_link->ref));
> +
> +  return ref_chain;
> +}
> +
> +
>  /* Identify instruction AND with identical zero extension.  */
>  
>  static unsigned int
> @@ -393,6 +443,7 @@ combine_set_extension (ext_cand *cand, rtx_insn *curr_insn, rtx *orig_set)
>    machine_mode orig_mode = GET_MODE (SET_DEST (*orig_set));
>    rtx new_set = NULL_RTX;
>    rtx cand_pat = single_set (cand->insn);
> +

superfluous whitespace change.

>    if (insn_is_zext_p(cand->insn)
>         && CONST_INT_P (orig_src) && INTVAL (orig_src) != 0)
>      return false;
> @@ -401,6 +452,7 @@ combine_set_extension (ext_cand *cand, rtx_insn *curr_insn, rtx *orig_set)
>       then we need to change the original load to reference the destination
>       of the extension.  Then we need to emit a copy from that destination
>       to the original destination of the load.  */
> + // print_rtl_single(stdout, cand_pat);

Please remove that debug comment.

>    rtx new_reg;
>    bool copy_needed
>      = (REGNO (SET_DEST (cand_pat)) != REGNO (XEXP (SET_SRC (cand_pat), 0)));
> @@ -547,6 +599,7 @@ transform_ifelse (ext_cand *cand, rtx_insn *def_insn)
>    return false;
>  }
>  
> +#if 0
>  /* Get all the reaching definitions of an instruction.  The definitions are
>     desired for REG used in INSN.  Return the definition list or NULL if a
>     definition is missing.  If DEST is non-NULL, additionally push the INSN
> @@ -593,7 +646,7 @@ get_defs (rtx_insn *insn, rtx reg, vec<rtx_insn *> *dest)
>  
>    return ref_chain;
>  }
> -
> +#endif

Why did you move get_defs?

>  /* Get all the reaching uses of an instruction.  The uses are desired for REG
>     set in INSN.  Return use list or NULL if a use is missing or irregular.  */
>  
> @@ -848,6 +901,36 @@ is_feasible_elim_across_basic_blocks (ext_cand *cand,
>  	&& REGNO (XEXP (PATTERN (BB_END (bb)), 0)) != REGNO (SET_DEST (cand->expr)))
>       return false;
>  
> +   if (cand->code == ZERO_EXTEND
> +	&& GET_CODE ((PATTERN (def_insn))) == PARALLEL)
> +     return false;

Excess braces above.

> +
> +   if (cand->code == ZERO_EXTEND
> +	&& GET_CODE ((PATTERN (def_insn))) == SET

Excess braces above.

> +	&& GET_CODE (SET_SRC (PATTERN (def_insn))) != XOR)
> +     return false;
> +
> +   if (cand->code == ZERO_EXTEND
> +	&& GET_CODE ((PATTERN (def_insn))) == SET
> +	&& GET_CODE (SET_SRC (PATTERN (def_insn))) == XOR)
> +     {
> +	vec<rtx_insn *> *dest = XCNEWVEC (vec<rtx_insn *>, 4);
> +	if (!get_defs (def_insn, XEXP (SET_SRC (PATTERN(def_insn)), 0), dest))
> +	  return false;

missing space between PATTERN and the brace

This leaks dest.

The above looks a bit redundant.
I'd have a single test for cand->code == ZERO_EXTEND and then
  rtx def = PATTERN (def_insn);

  if (def == PARALLEL)
    return false;
  if (def == SET)
    {
	rtx src = SET_SRC (def);
	if (GET_CODE (src) != XOR)
	  return false;
	/* maybe use an auto_vec here */
...


> +
> +	int i;
> +	rtx_insn *def_insn;
> +
> +	FOR_EACH_VEC_ELT (*dest, i, def_insn)
> +	  {
> +	    if ((GET_CODE (PATTERN (def_insn)) == SET
> +		&& GET_CODE (SET_SRC (PATTERN (def_insn))) == ASHIFT)
> +		|| GET_CODE (PATTERN (def_insn)) == PARALLEL)

leaks dest.

> +	       return false;
> +	  }
> +	XDELETEVEC (dest);
> +      }
> +
>     return true;
>  }
>  
> @@ -873,7 +956,8 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn, ext_state *state)
>  
>    if (!feasible) return false;
>  
> -  if (((!copy_needed && (insn_is_zext_p (cand->insn))
> +  if (((!copy_needed && (insn_is_zext_p (cand->insn)
> +	|| (cand->code == ZERO_EXTEND && ext_src_mode == QImode))
>  	&& (GET_MODE (SET_DEST (*sub_rtx)) != ext_src_mode
>  	&& state->modified[INSN_UID (def_insn)].kind == EXT_MODIFIED_NONE))
>  	|| ((state->modified[INSN_UID (def_insn)].kind
> @@ -1211,15 +1295,22 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>  	 definitions could be merged.  */
>        if (apply_change_group ())
>          {
> -          if (dump_file)
> -            fprintf (dump_file, "All merges were successful.\n");
> +	  if (state->modified_list.length() == 0) return false;

Missing space after length before the opening brace, and missing
newline before return.

> +
> +	  if (cand->code == ZERO_EXTEND
> +	      && GET_CODE (PATTERN (state->modified_list[0])) == SET
> +	      && GET_CODE (SET_SRC (PATTERN (state->modified_list[0]))) != XOR)
> +	     return false;
> +
> +	   if (dump_file)
> +	     fprintf (dump_file, "All merges were successful.\n");
>  
>  	  FOR_EACH_VEC_ELT (state->modified_list, i, def_insn)
>  	    {
>  	      ext_modified *modified = &state->modified[INSN_UID (def_insn)];
>  	      if (modified->kind == EXT_MODIFIED_NONE)
>  		modified->kind = (cand->code == ZERO_EXTEND  ? EXT_MODIFIED_ZEXT
> -						            : EXT_MODIFIED_SEXT);
> +							     : EXT_MODIFIED_SEXT);
>  
>  	      if (copy_needed)
>  		modified->do_not_reextend = 1;
> @@ -1228,6 +1319,26 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>          }
>  	else
>  	  {
> +	    if (state->modified_list.length() == 0) return false;

same comment as above

> +
> +	    if (cand->code == ZERO_EXTEND
> +		&& GET_CODE (PATTERN(state->modified_list[0])) == SET
> +		&& GET_CODE (SET_SRC (PATTERN (state->modified_list[0]))) != XOR)
> +	      return false;

Missing space before '('

> +
> +	    if (cand->code == ZERO_EXTEND)
> +	      {
> +		FOR_EACH_VEC_ELT (state->modified_list, i, def_insn)
> +		  {
> +		    ext_modified *modified = &state->modified[INSN_UID (def_insn)];
> +		    if (modified->kind == EXT_MODIFIED_NONE)
> +		      modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT
> +								  : EXT_MODIFIED_SEXT);

The whole loop is guarded by code == ZERO_EXTEND, so the rhs condition
above is redundant.
thanks,

> +
> +		     modified->do_not_reextend = 1;
> +		   }
> +		  return true;
> +	      }
>  	    /* Changes need not be cancelled explicitly as apply_change_group
>  		does it.  Print list of definitions in the dump_file for debug
>  		purposes.  This extension cannot be deleted.  */