public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Wilco Dijkstra <Wilco.Dijkstra@arm.com>
To: Richard Earnshaw <Richard.Earnshaw@arm.com>,
	Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: [PATCH] ARM: Fix conditional execution [PR113915]
Date: Wed, 21 Feb 2024 14:34:42 +0000	[thread overview]
Message-ID: <PAWPR08MB8982C55211A0F7A7C1813D1F83572@PAWPR08MB8982.eurprd08.prod.outlook.com> (raw)


By default most patterns can be conditionalized on Arm targets.  However
Thumb-2 predication requires the "predicable" attribute be explicitly
set to "yes".  Most patterns are shared between Arm and Thumb(-2) and are
marked with "predicable".  Given this sharing, it does not make sense to
use a different default for Arm.  So only consider conditional execution
of instructions that have the predicable attribute set to yes.  This ensures
that patterns not explicitly marked as such are never accidentally 
conditionally executed like in the PR.

GLIBC codesize was ~0.014% worse due to atomic operations now being
unconditional and a small number of patterns not setting "predicable".

Passes regress and bootstrap, OK for commit?

gcc/ChangeLog:
        PR target/113915
        * config/arm/arm.md (NOCOND): Improve comment.
        * config/arm/arm.cc (arm_final_prescan_insn): Add check for
        PREDICABLE_YES.

gcc/testsuite/ChangeLog:
        PR target/113915
        * gcc.target/arm/builtin-bswap-1.c: Fix test.

---

diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index c44047c377a802d0c1dc1406df1b88a6b079607b..29771d284831a995adcf9adbb525396fbabb1ea2 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -25610,11 +25610,12 @@ arm_final_prescan_insn (rtx_insn *insn)
 	      break;
 
 	    case INSN:
-	      /* Instructions using or affecting the condition codes make it
-		 fail.  */
+	      /* Check the instruction is explicitly marked as predicable.
+		 Instructions using or affecting the condition codes are not.  */
 	      scanbody = PATTERN (this_insn);
 	      if (!(GET_CODE (scanbody) == SET
 		    || GET_CODE (scanbody) == PARALLEL)
+		  || get_attr_predicable (this_insn) != PREDICABLE_YES
 		  || get_attr_conds (this_insn) != CONDS_NOCOND)
 		fail = TRUE;
 	      break;
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 5816409f86f1106b410c5e21d77e599b485f85f2..671f093862259c2c0df93a986fc22fa56a8ea6c7 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -307,6 +307,8 @@
 ;
 ; NOCOND means that the instruction does not use or alter the condition
 ;   codes but can be converted into a conditionally exectuted instruction.
+;   Given that NOCOND is the default for most instructions if omitted,
+;   the attribute predicable must be set to yes as well.
 
 (define_attr "conds" "use,set,clob,unconditional,nocond"
 	(if_then_else
diff --git a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c
index c1e7740d14d3ca4e93a71e38b12f82c19791a204..3de7cea81c1128c2fe5a9e1216e6b027d26bcab9 100644
--- a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c
+++ b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c
@@ -5,14 +5,8 @@
    of the instructions.  Add an -mtune option known to facilitate that.  */
 /* { dg-additional-options "-O2 -mtune=cortex-a53" } */
 /* { dg-final { scan-assembler-not "orr\[ \t\]" } } */
-/* { dg-final { scan-assembler-times "revsh\\t" 1 { target { arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "revshne\\t" 1 { target { arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "revsh\\t" 2 { target { ! arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "rev16\\t" 1 { target { arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "rev16ne\\t" 1 { target { arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "rev16\\t" 2 { target { ! arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "rev\\t" 2 { target { arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "revne\\t" 2 { target { arm_nothumb } } } }  */
-/* { dg-final { scan-assembler-times "rev\\t" 4 { target { ! arm_nothumb } } } }  */
+/* { dg-final { scan-assembler-times "revsh\\t" 2 } }  */
+/* { dg-final { scan-assembler-times "rev16\\t" 2 } }  */
+/* { dg-final { scan-assembler-times "rev\\t" 4 } }  */
 
 #include "builtin-bswap.x"


             reply	other threads:[~2024-02-21 14:34 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-21 14:34 Wilco Dijkstra [this message]
2024-02-21 15:58 ` Richard Earnshaw (lists)
2024-02-23 15:46   ` Wilco Dijkstra
2024-02-26 15:58     ` Richard Earnshaw (lists)
2024-02-26 16:05       ` Wilco Dijkstra
2024-02-26 16:13         ` Richard Earnshaw (lists)

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=PAWPR08MB8982C55211A0F7A7C1813D1F83572@PAWPR08MB8982.eurprd08.prod.outlook.com \
    --to=wilco.dijkstra@arm.com \
    --cc=Kyrylo.Tkachov@arm.com \
    --cc=Richard.Earnshaw@arm.com \
    --cc=gcc-patches@gcc.gnu.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).