Re: [PATCH] rtl: add predicates for addition, subtraction & multiplication

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

Hi!

On Sat, Nov 26, 2022 at 09:16:13PM -0500, Charlie Sale via Gcc-patches wrote:
> This is my first contribution to GCC :) one of the beginner projects
> suggested on the website was to add and use RTL type predicates.

It says "See which ones are worth having a predicate for, and add them."

None of the operations should get a predicate, imnsho, only more
structural things.  Code using PLUS_P is way *less* readable than that
using GET_CODE directly!  It is good if important things are more
explicit, it is bad to have many names, etc.

> +	* rtl.h (PLUS_P): RTL addition predicate
> +	(MINUS_P): RTL subtraction predicate
> +	(MULT_P): RTL multiplication predicate

	* rtl.h (PLUS_P): New.

> +	* alias.cc: use RTL predicates

	* alias.cc: Use new predicates.

Send the changelog as plain text btw, not as patch; if nothing else,
such patches will never apply cleanly :-)

>  			  set_reg_known_value (regno, XEXP (note, 0));
> -			  set_reg_known_equiv_p (regno,
> -						 REG_NOTE_KIND (note) == REG_EQUIV);
> +			  set_reg_known_equiv_p (regno, REG_NOTE_KIND (note)
> +							  == REG_EQUIV);

Don't reformat unrelated code.  And certainly not to something that
violates our coding standards :-)

> -			       && (t = get_reg_known_value (REGNO (XEXP (src, 0))))
> +			       && (t = get_reg_known_value (
> +				     REGNO (XEXP (src, 0))))

Wow, this is even worse.  Why would you do this at all?  I guess you
used some automatic formatting thing that sets maximum line length to
79?  It is 80, and it is a bad idea to reformat any random code.

> -	   && (REG_P (XEXP (SET_SRC (pat), 1)))
> -	   && GET_CODE (SET_SRC (pat)) == PLUS)
> +	   && (REG_P (XEXP (SET_SRC (pat), 1))) && PLUS_P (SET_SRC (pat)))

You could have removed the superfluous extra parentheses here :-)

>  	case SUBREG:
>  	  if ((SUBREG_PROMOTED_VAR_P (x)
>  	       || (REG_P (SUBREG_REG (x)) && REG_POINTER (SUBREG_REG (x)))
> -	       || (GET_CODE (SUBREG_REG (x)) == PLUS
> -		   && REG_P (XEXP (SUBREG_REG (x), 0))
> +	       || (PLUS_P (SUBREG_REG (x)) && REG_P (XEXP (SUBREG_REG (x), 0))
>  		   && REG_POINTER (XEXP (SUBREG_REG (x), 0))
>  		   && CONST_INT_P (XEXP (SUBREG_REG (x), 1))))

There was only one && per line here on purpose.  It makes the code much
more readable.

> -  if (GET_CODE (x) == PLUS
> -      && XEXP (x, 0) == stack_pointer_rtx
> +  if (PLUS_P (x) && XEXP (x, 0) == stack_pointer_rtx
>        && CONST_INT_P (XEXP (x, 1)))

Similar here (but it is so simple here that either is easy to read of
course).

> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -3016,19 +3016,17 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
>    /* See if any of the insns is a MULT operation.  Unless one is, we will
>       reject a combination that is, since it must be slower.  Be conservative
>       here.  */
> -  if (GET_CODE (i2src) == MULT
> -      || (i1 != 0 && GET_CODE (i1src) == MULT)
> -      || (i0 != 0 && GET_CODE (i0src) == MULT)
> -      || (GET_CODE (PATTERN (i3)) == SET
> -	  && GET_CODE (SET_SRC (PATTERN (i3))) == MULT))
> +  if (MULT_P (i2src) || (i1 != 0 && MULT_P (i1src))
> +      || (i0 != 0 && MULT_P (i0src))
> +      || (GET_CODE (PATTERN (i3)) == SET && MULT_P (SET_SRC (PATTERN (i3)))))
>      have_mult = 1;

No.  All || align here.  Please leave it that way.

> -  /* If I3 has an inc, then give up if I1 or I2 uses the reg that is inc'd.
> -     We used to do this EXCEPT in one case: I3 has a post-inc in an
> -     output operand.  However, that exception can give rise to insns like
> -	mov r3,(r3)+
> -     which is a famous insn on the PDP-11 where the value of r3 used as the
> -     source was model-dependent.  Avoid this sort of thing.  */
> +    /* If I3 has an inc, then give up if I1 or I2 uses the reg that is inc'd.
> +       We used to do this EXCEPT in one case: I3 has a post-inc in an
> +       output operand.  However, that exception can give rise to insns like
> +	  mov r3,(r3)+
> +       which is a famous insn on the PDP-11 where the value of r3 used as the
> +       source was model-dependent.  Avoid this sort of thing.  */

The indentation was correct, it now isn't anymore.  There is absolutely
no reason to touch this at all anyway.  NAK.

> -  if ((FIND_REG_INC_NOTE (i2, NULL_RTX) != 0
> -       && i2_is_used + added_sets_2 > 1)
> +  if ((FIND_REG_INC_NOTE (i2, NULL_RTX) != 0 && i2_is_used + added_sets_2 > 1)

Do not touch random other code please.  If there is a reason to reformat
it (there isn't here!) do that as a separate patch.

>        || (i1 != 0 && FIND_REG_INC_NOTE (i1, NULL_RTX) != 0
> -	  && (i1_is_used + added_sets_1 + (added_sets_2 && i1_feeds_i2_n)
> -	      > 1))
> +	  && (i1_is_used + added_sets_1 + (added_sets_2 && i1_feeds_i2_n) > 1))
>        || (i0 != 0 && FIND_REG_INC_NOTE (i0, NULL_RTX) != 0
> -	  && (n_occurrences + added_sets_0
> -	      + (added_sets_1 && i0_feeds_i1_n)
> -	      + (added_sets_2 && i0_feeds_i2_n)
> +	  && (n_occurrences + added_sets_0 + (added_sets_1 && i0_feeds_i1_n)
> +		+ (added_sets_2 && i0_feeds_i2_n)
>  	      > 1))

These are way worse than they were before, and also different layou than
we use in GCC.  NAK.

> -      if (GET_CODE (XEXP (x, 0)) == PLUS
> -	  && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> -	  && ! memory_address_addr_space_p (GET_MODE (x), XEXP (x, 0),
> -					    MEM_ADDR_SPACE (x)))
> +      if (PLUS_P (XEXP (x, 0)) && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> +	  && !memory_address_addr_space_p (GET_MODE (x), XEXP (x, 0),
> +					   MEM_ADDR_SPACE (x)))

combine.c (as well as other older code) uses space-after-bang a lot.
Do not change that randomly please, because that a) makes it harder to
review the code (separate patch please!), and b) it makes it harder to
read the resulting code!  Or did you change do s/! ([a-zA-A0-9_])/!\1/
here on the whole file?

> -	  rtx_insn *seq = combine_split_insns (gen_rtx_SET (reg, XEXP (x, 0)),
> -					       subst_insn);
> +	  rtx_insn *seq
> +	    = combine_split_insns (gen_rtx_SET (reg, XEXP (x, 0)), subst_insn);

The original was better.

For big mechanical patches, please do only *one* thing per patch, so all
the patches in the series will be totally boring and much MUCH easier to
review.  But again, I think it is a mistake to make predicates for PLUS
and MINUS and such.  Sorry.  PLUS is frequent in this patch, but MINUS
and MULT are not at all, and even PLUS is not common in our code.  A few
characters shorter is nice for *very* common things, but extra
expressiveness through verbosity is worth more here, imo.


Segher