From: "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>
To: Wilco Dijkstra <Wilco.Dijkstra@arm.com>,
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] ARM: Fix conditional execution [PR113915]
Date: Wed, 21 Feb 2024 15:58:11 +0000 [thread overview]
Message-ID: <d4dcd4a0-e99c-46a5-8ddb-38842688985c@arm.com> (raw)
In-Reply-To: <PAWPR08MB8982C55211A0F7A7C1813D1F83572@PAWPR08MB8982.eurprd08.prod.outlook.com>
On 21/02/2024 14:34, Wilco Dijkstra wrote:
>
> 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
While this is ok,
> 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"
This bit isn't. The correct fix here is to fix the pattern(s) concerned to add the missing predicate.
Note that builtin-bswap.x explicitly mentions predicated mnemonics in the comments.
R.
next prev parent reply other threads:[~2024-02-21 15:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-21 14:34 Wilco Dijkstra
2024-02-21 15:58 ` Richard Earnshaw (lists) [this message]
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=d4dcd4a0-e99c-46a5-8ddb-38842688985c@arm.com \
--to=richard.earnshaw@arm.com \
--cc=Kyrylo.Tkachov@arm.com \
--cc=Wilco.Dijkstra@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).