Re: [PATCH] rs6000: Canonicalize copysign (x, -1) back to -abs (x) in the backend [PR112606]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Jakub,

on 2023/11/25 18:17, Jakub Jelinek wrote:
> Hi!
> 
> The middle-end has been changed quite recently to canonicalize
> -abs (x) to copysign (x, -1) rather than the other way around.
> While I agree with that at GIMPLE level, since it matches the GIMPLE
> goal of as few operations as possible for a canonical form (-abs (x)
> is 2 GIMPLE statements, copysign (x, -1) is just one), I must say
> I don't really like that being done on RTL as well (or at least
> not canonicalizing (COPYSIGN x, negative) back to (NEG (ABS x))),
> because on most targets most of floating point constants need to be loaded
> from memory, there are a few exceptions but -1 is often not one of them.
> 
> Anyway, the following patch fixes the rs6000 regression caused by the
> change in GIMPLE canonicalization (i.e. the desirable one).  As rs6000
> clearly prefers -abs (x) form because it has a single instruction to do
> that while it also has copysign instruction, but that requires loading the
> -1 from memory, the following patch just ensures the copysign expander
> can actually see the floating point constant and in that case emits the
> -abs (x) code (or in the hypothetical case of copysign with non-negative
> constant abs (x) - but there copysign (x, 1) in GIMPLE is canonicalized
> to abs (x)), otherwise forces the operand to be the expected gpc_reg_operand
> and does what it did before.
> 
> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

Thanks for fixing this!  IIUC even with Tamar's further improvement proposal,
we still need some rs6000 specific work, then updating this copysign expansion
looks more straightforward.  So okay for trunk, thanks!

BR,
Kewen

> 
> 2023-11-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/112606
> 	* config/rs6000/rs6000.md (copysign<mode>3): Change predicate
> 	of the last argument from gpc_reg_operand to any_operand.  If
> 	operands[2] is CONST_DOUBLE, emit abs or neg abs depending on
> 	its sign, otherwise if it doesn't satisfy gpc_reg_operand,
> 	force it to REG using copy_to_mode_reg.
> 
> --- gcc/config/rs6000/rs6000.md.jj	2023-10-13 19:34:43.927834877 +0200
> +++ gcc/config/rs6000/rs6000.md	2023-11-24 18:54:13.587876170 +0100
> @@ -5358,7 +5358,7 @@ (define_expand "copysign<mode>3"
>     (set (match_dup 4)
>  	(neg:SFDF (abs:SFDF (match_dup 1))))
>     (set (match_operand:SFDF 0 "gpc_reg_operand")
> -        (if_then_else:SFDF (ge (match_operand:SFDF 2 "gpc_reg_operand")
> +	(if_then_else:SFDF (ge (match_operand:SFDF 2 "any_operand")
>  			       (match_dup 5))
>  			 (match_dup 3)
>  			 (match_dup 4)))]
> @@ -5369,6 +5369,24 @@ (define_expand "copysign<mode>3"
>         || TARGET_CMPB
>         || VECTOR_UNIT_VSX_P (<MODE>mode))"
>  {
> +  /* Middle-end canonicalizes -fabs (x) to copysign (x, -1),
> +     but PowerPC prefers -fabs (x).  */
> +  if (CONST_DOUBLE_AS_FLOAT_P (operands[2]))
> +    {
> +      if (real_isneg (CONST_DOUBLE_REAL_VALUE (operands[2])))
> +	{
> +	  operands[3] = gen_reg_rtx (<MODE>mode);
> +	  emit_insn (gen_abs<mode>2 (operands[3], operands[1]));
> +	  emit_insn (gen_neg<mode>2 (operands[0], operands[3]));
> +	}
> +      else
> +	emit_insn (gen_abs<mode>2 (operands[0], operands[1]));
> +      DONE;
> +    }
> +
> +  if (!gpc_reg_operand (operands[2], <MODE>mode))
> +    operands[2] = copy_to_mode_reg (<MODE>mode, operands[2]);
> +
>    if (TARGET_CMPB || VECTOR_UNIT_VSX_P (<MODE>mode))
>      {
>        emit_insn (gen_copysign<mode>3_fcpsgn (operands[0], operands[1],
> 
> 	Jakub
>