public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Segher Boessenkool <segher@kernel.crashing.org>,
	David Edelsohn <dje.gcc@gmail.com>
Cc: gcc-patches@gcc.gnu.org,
	Tamar Christina <Tamar.Christina@arm.com>,
	Andrew Pinski <apinski@marvell.com>
Subject: [PATCH] rs6000: Canonicalize copysign (x, -1) back to -abs (x) in the backend [PR112606]
Date: Sat, 25 Nov 2023 11:17:48 +0100	[thread overview]
Message-ID: <ZWHJzN4hJHFSZ28f@tucnak> (raw)

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?

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


             reply	other threads:[~2023-11-25 10:18 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-25 10:17 Jakub Jelinek [this message]
2023-11-25 10:42 ` Xi Ruoyao
2023-11-25 12:03   ` Tamar Christina
2023-11-25 12:09     ` Jakub Jelinek
2023-11-27  7:55       ` Tamar Christina
2023-11-27  8:12         ` Jakub Jelinek
2023-11-27  8:22           ` Tamar Christina
2023-12-04  8:35 ` Patch ping: " Jakub Jelinek
2023-12-04  9:21 ` Kewen.Lin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZWHJzN4hJHFSZ28f@tucnak \
    --to=jakub@redhat.com \
    --cc=Tamar.Christina@arm.com \
    --cc=apinski@marvell.com \
    --cc=dje.gcc@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=segher@kernel.crashing.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).