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

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Tue, Mar 28, 2023 at 10:19:27PM +0530, Ajit Agarwal wrote:
> This patch makes REE pass as a default pass in rs6000 target. And
> add necessary subroutines to eliminate extensions across basic blocks.

Please wait with this until stage 1?

Some comments:

> 	rtl-optimization: ppc backend generates unnecessary
>  	extension.

Subject can start with "ree: ", but not "rtl-optimization: ".  Subject
should start with a capital (the part after the colon, that is).  There
should not be a full stop in the subject.

Subjects should be in the mail subject, not elsewhere in the patch.

> 	Eliminate unnecessary redundant zero extension across basic
> 	blocks.

Not sure where you want this?  It doesn't belong in the changelog.

> 	* ree.cc(insn_s_zext_p): New function.

Space before (.

> 	* ree.cc(is_feasible_elim_across_basic_blocks):
> 	New function.

This is the same file as the one before this.  You write that as
	(is_feasible_elim_across_basic_blocks): New function.

(and no early line breaks please, certainly not after a colon).

> 	* common/config/rs6000/rs6000-common.cc: Add free pass
> 	as default pass in rs6000 target.

Changelog lines are 80 positions.  The leading tab is 8 positions.

> @@ -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 },

>      /* Split multi-word types early.  */
>      { OPT_LEVELS_ALL, OPT_fsplit_wide_types_early, NULL, 1 },
>      /* Enable -fsched-pressure for first pass instruction scheduling.  */
> @@ -42,7 +44,6 @@ static const struct default_options rs6000_option_optimization_table[] =
>      /* -frename-registers leads to non-optimal codegen and performance
>         on rs6000, turn it off by default.  */
>      { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
> -

Don't remove random lines please.  Never do this as part of unrelated
patches, too.

> +/* Identify instruction AND with identical zero extension.  */

What does that mean?

> +
> +static unsigned int
> +insn_is_zext_p(rtx insn)

Space before (.  If it is an insn, you probably want rtx_insn?

> +{
> +  if (GET_CODE (insn) == AND)
> +   {

Wrong indentation.

> +     rtx set = XEXP (insn, 0);
> +     if (REG_P(set))
> +       {
> +	 if (CONST_INT_P (XEXP (insn, 1))

You can see that here: all indentations are a multiple of two, so a
single space indent is always wrong.

> +	     && INTVAL (XEXP (insn, 1)) == 1)

This easily fits on the previous line.

> +	   return 1;
> +       }
> +     else
> +       return 0;
> +   }
> +
> +  return 0;
> +}

Don't "else return 0" if you have a catch-all anyway?

> +/* Identify instruction AND with identical zero extension.  */
> +
> +static unsigned int
> +insn_is_zext_p(rtx_insn * insn)

No space after * for a pointer.

> +{
> +  rtx body = PATTERN (insn);
> +
> +  if (GET_CODE (body) == PARALLEL) return 0;

Always a new line before an "if" body.

> +  if (GET_CODE(body) == SET && GET_CODE (SET_SRC (body)) == AND)
> +   {
> +     rtx set = XEXP (SET_SRC(body), 0);
> +
> +     if (REG_P(set) && GET_MODE(SET_DEST(body))
> +			 == GET_MODE(set))

What is this for?

> +       {
> +	 if (CONST_INT_P (XEXP (SET_SRC (body), 1))
> +	      && INTVAL (XEXP (SET_SRC (body), 1)) == 1)

The && should align with the previous "C".

You could just say
  if (XEXP (SET_SRC (body), 1) == const1_rtx)
.

> +  if (insn_is_zext_p(cand->insn)
> +       && CONST_INT_P (orig_src) && INTVAL (orig_src) != 0)
> +    return false;

  if (insn_is_zext_p (cand->insn) && orig_src != const0_rtx)
    return false;


So what is this patch doing?  At least two things, right?  It aims to
improve what REE does, and also enables it by default for rs6000?  So
make it two patches, please.

Can you talk a bit about what improvements to generated code you see?


Segher