Re: [PATCH] ree: Improvement of ree pass for rs6000 target.

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Apr 04, 2023 at 05:02:35PM +0530, Ajit Agarwal via Gcc-patches wrote:
> This patch eliminates unnecessary redundant extension within basic and across basic blocks. For rs6000 target we see
> redundant zero and sign extension and done to improve ree pass to eliminate such redundant zero and sign extension.

Just random comments, will defer the rest to whomever reviews it for stage1.
> 	testsuite/g++.target/powerpc/zext-elim.C: New testcase.
> 	testsuite/g++.target/powerpc/sext-elim.C: New testcase.

Missing * and space before the filenames, plus testsuite has a separate
ChangeLog, so it should be
gcc/testsuite/ChangeLog
	* g++.target/powerpc/zext-elim.C: New testcase.
etc.
> --- a/gcc/common/config/rs6000/rs6000-common.cc
> +++ b/gcc/common/config/rs6000/rs6000-common.cc
> @@ -30,6 +30,8 @@
>  /* Implement TARGET_OPTION_OPTIMIZATION_TABLE.  */
>  static const struct default_options rs6000_option_optimization_table[] =
>    {
> +    /* Enable -free for zero extension and sign extension elimination.*/
> +    { OPT_LEVELS_2_PLUS, OPT_free, NULL, 1 },

I believe the options should be sorted by the OPT_LEVEL* they are given.

>      /* Split multi-word types early.  */
>      { OPT_LEVELS_ALL, OPT_fsplit_wide_types_early, NULL, 1 },
>      /* Enable -fsched-pressure for first pass instruction scheduling.  */
> @@ -38,11 +40,9 @@ static const struct default_options rs6000_option_optimization_table[] =
>         loops at -O2 and above by default.  */
>      { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
>      { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL, 1 },
> -
>      /* -frename-registers leads to non-optimal codegen and performance
>         on rs6000, turn it off by default.  */
>      { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
> -
>      /* Double growth factor to counter reduced min jump length.  */
>      { OPT_LEVELS_ALL, OPT__param_max_grow_copy_bb_insns_, NULL, 16 },
>      { OPT_LEVELS_NONE, 0, NULL, 0 }

Why?

> +     rtx set = XEXP (SET_SRC(body), 0);

Space missing before (body
The indentation is also incorrect, it should be always in multiplies
of 2, the above is 5 columns.

> +
> +     if (REG_P(set) && GET_MODE(SET_DEST(body))
> +			 == GET_MODE(set))

Similarly.  Furthermore, the formatting is wrong.  In this case, there is no
reason why
      if (REG_P (set) && GET_MODE (SET_DEST (body)) == GET_MODE (set))
wouldn't fit on one line, but if it would be longer, either one would use
  if (whatever
      && whatever_else)
or
  if (whatever && (whatever_else
		   == something))
or similar.
> +      && GET_CODE (SET_DEST (expr)) == REG

Use REG_P (SET_DEST (expr))

> +      && GET_CODE (SET_SRC (expr))  == IF_THEN_ELSE
> +      && GET_CODE (XEXP (SET_SRC (expr), 1)) == REG
> +      && GET_CODE (XEXP (SET_SRC (expr), 2)) == REG)

Similarly.

> +    {
> +      *reg1 = XEXP (SET_SRC (expr), 1);
> +      *reg2 = XEXP (SET_SRC (expr), 2);
> +      return true;
> +    }

> @@ -319,9 +440,8 @@ combine_set_extension (ext_cand *cand, rtx_insn *curr_insn, rtx *orig_set)
>  {
>    rtx orig_src = SET_SRC (*orig_set);
>    machine_mode orig_mode = GET_MODE (SET_DEST (*orig_set));
> -  rtx new_set;
> +  rtx new_set = NULL_RTX;
>    rtx cand_pat = single_set (cand->insn);
> -

Why this random whitespace change?

> @@ -734,8 +1006,7 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn, ext_state *state)
>  	  return true;
>  	}
>      }
> -
> -  return false;
> +    return false;
>  }

The old code was properly formatted, this one isn't.

>  /* Given SRC, which should be one or more extensions of a REG, strip
> @@ -744,7 +1015,8 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn, ext_state *state)
>  static inline rtx
>  get_extended_src_reg (rtx src)
>  {
> -  while (GET_CODE (src) == SIGN_EXTEND || GET_CODE (src) == ZERO_EXTEND)
> +  while (GET_CODE (src) == SIGN_EXTEND || GET_CODE (src) == ZERO_EXTEND
> +	|| insn_is_zext_p(src))

If a condition doesn't fit into one line, then the &&/|| terms should be
split into one term on one line, so
  while (GET_CODE (src) == SIGN_EXTEND
	 || GET_CODE (src) == ZERO_EXTEND
	 || insn_is_zext_p (src))
and note again missing space.

>      src = XEXP (src, 0);
>    gcc_assert (REG_P (src));
>    return src;
> @@ -767,10 +1039,48 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
>    int i;
>    int defs_ix;
>    bool outcome;
> -

Again, why?  Don't do this.
>    state->defs_list.truncate (0);
>    state->copies_list.truncate (0);
>  
> +  if (cand->code == ZERO_EXTEND)
> +    {
> +      rtx orig_src = XEXP (SET_SRC (cand->expr),0);
> +      if (!get_defs (cand->insn, orig_src, NULL))
> +	{
> +	  if (GET_MODE (orig_src) == QImode
> +	      && FUNCTION_ARG_REGNO_P (REGNO (orig_src))
> +	      && !FUNCTION_VALUE_REGNO_P (REGNO (orig_src)))
> +	    {
> +	      if (side_effects_p (PATTERN (cand->insn)))
> +		return false;
> +
> +	      struct df_link *uses
> +		 = get_uses (cand->insn, SET_DEST (PATTERN (cand->insn)));
> +
> +	      if (!uses) return false;
> +
> +	      for (df_link *use = uses; use; use = use->next)
> +		{
> +		  if (BLOCK_FOR_INSN (cand->insn)
> +		      != BLOCK_FOR_INSN (DF_REF_INSN (use->ref)))
> +		    return false;
> +
> +		  rtx_insn *insn = DF_REF_INSN (use->ref);
> +
> +		  if (GET_CODE (PATTERN (insn)) == SET)
> +		    {
> +		      rtx_code code = GET_CODE (SET_SRC (PATTERN (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;
> +	    }
> +	}
> +     }

Again, indent by 5 spaces can't be right.

> -		modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT
> -						            : EXT_MODIFIED_SEXT);
> +		modified->kind = (cand->code == ZERO_EXTEND  ? EXT_MODIFIED_ZEXT
> +							     : EXT_MODIFIED_SEXT);

Why?  Old code was properly formatted, this one isn't.
> @@ -1368,9 +1730,11 @@ find_and_remove_re (void)
>    reinsn_list.release ();
>    XDELETEVEC (state.modified);
>  
> -  if (dump_file && num_re_opportunities > 0)
> +  if (dump_file  &&  num_re_opportunities > 0) {
>      fprintf (dump_file, "Elimination opportunities = %d realized = %d\n",
>  	     num_re_opportunities, num_realized);
> +  }
> +
>  }

Again, why?  This hunk turns something properly formatted into something
with multiple formatting issues in it.

	Jakub