| Message ID | PAWPR08MB8982C55211A0F7A7C1813D1F83572@PAWPR08MB8982.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: Fix conditional execution [PR113915] | expand |
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.
Hi Richard, > 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. I fixed the patterns in v2. There are likely some more, plus we could likely merge many t1 and t2 patterns where the only difference is predication. But those cleanups are for another time... Cheers, Wilco v2: Add predicable to the rev patterns. 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 conditionally executed. Passes regress and bootstrap, OK for commit? gcc/ChangeLog: PR target/113915 * config/arm/arm.md (NOCOND): Improve comment. (arm_rev*) Add predicable. * 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 1cd69268ee986a0953cc85ab259355d2191250ac..6a35fe44138135998877a9fb74c2a82a7f99dcd5 100644 --- a/gcc/config/arm/arm.cc +++ b/gcc/config/arm/arm.cc @@ -25613,11 +25613,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..81237a61d4a2ebcfb77e47c2bd29137aba28a521 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 @@ -12547,6 +12549,7 @@ revsh%?\t%0, %1" [(set_attr "arch" "t1,t2,32") (set_attr "length" "2,2,4") + (set_attr "predicable" "no,yes,yes") (set_attr "type" "rev")] ) @@ -12560,6 +12563,7 @@ rev16%?\t%0, %1" [(set_attr "arch" "t1,t2,32") (set_attr "length" "2,2,4") + (set_attr "predicable" "no,yes,yes") (set_attr "type" "rev")] ) @@ -12584,6 +12588,7 @@ rev16%?\t%0, %1" [(set_attr "arch" "t1,t2,32") (set_attr "length" "2,2,4") + (set_attr "predicable" "no,yes,yes") (set_attr "type" "rev")] ) @@ -12619,6 +12624,7 @@ rev16%?\t%0, %1" [(set_attr "arch" "t1,t2,32") (set_attr "length" "2,2,4") + (set_attr "predicable" "no,yes,yes") (set_attr "type" "rev")] ) diff --git a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c index c1e7740d14d3ca4e93a71e38b12f82c19791a204..1a311a6a5af647d40abd553e5d0ba1273c76d288 100644 --- a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c +++ b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c @@ -5,14 +5,11 @@ 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" 1 } } */ +/* { dg-final { scan-assembler-times "revshne\\t" 1 } } */ +/* { dg-final { scan-assembler-times "rev16\\t" 1 } } */ +/* { dg-final { scan-assembler-times "rev16ne\\t" 1 } } */ +/* { dg-final { scan-assembler-times "rev\\t" 2 } } */ +/* { dg-final { scan-assembler-times "revne\\t" 2 } } */ #include "builtin-bswap.x"
On 23/02/2024 15:46, Wilco Dijkstra wrote: > Hi Richard, > >> 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. > > I fixed the patterns in v2. There are likely some more, plus we could likely merge many t1 and t2 > patterns where the only difference is predication. But those cleanups are for another time... > > Cheers, > Wilco > > v2: Add predicable to the rev patterns. > > 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 conditionally executed. > > Passes regress and bootstrap, OK for commit? > > gcc/ChangeLog: > PR target/113915 > * config/arm/arm.md (NOCOND): Improve comment. > (arm_rev*) Add predicable. > * 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 1cd69268ee986a0953cc85ab259355d2191250ac..6a35fe44138135998877a9fb74c2a82a7f99dcd5 100644 > --- a/gcc/config/arm/arm.cc > +++ b/gcc/config/arm/arm.cc > @@ -25613,11 +25613,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..81237a61d4a2ebcfb77e47c2bd29137aba28a521 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 > @@ -12547,6 +12549,7 @@ > revsh%?\t%0, %1" > [(set_attr "arch" "t1,t2,32") > (set_attr "length" "2,2,4") > + (set_attr "predicable" "no,yes,yes") > (set_attr "type" "rev")] > ) > > @@ -12560,6 +12563,7 @@ > rev16%?\t%0, %1" > [(set_attr "arch" "t1,t2,32") > (set_attr "length" "2,2,4") > + (set_attr "predicable" "no,yes,yes") > (set_attr "type" "rev")] > ) > > @@ -12584,6 +12588,7 @@ > rev16%?\t%0, %1" > [(set_attr "arch" "t1,t2,32") > (set_attr "length" "2,2,4") > + (set_attr "predicable" "no,yes,yes") > (set_attr "type" "rev")] > ) > > @@ -12619,6 +12624,7 @@ > rev16%?\t%0, %1" > [(set_attr "arch" "t1,t2,32") > (set_attr "length" "2,2,4") > + (set_attr "predicable" "no,yes,yes") > (set_attr "type" "rev")] > ) > > diff --git a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c > index c1e7740d14d3ca4e93a71e38b12f82c19791a204..1a311a6a5af647d40abd553e5d0ba1273c76d288 100644 > --- a/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c > +++ b/gcc/testsuite/gcc.target/arm/builtin-bswap-1.c > @@ -5,14 +5,11 @@ > 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" 1 } } */ > +/* { dg-final { scan-assembler-times "revshne\\t" 1 } } */ > +/* { dg-final { scan-assembler-times "rev16\\t" 1 } } */ > +/* { dg-final { scan-assembler-times "rev16ne\\t" 1 } } */ > +/* { dg-final { scan-assembler-times "rev\\t" 2 } } */ > +/* { dg-final { scan-assembler-times "revne\\t" 2 } } */ > > #include "builtin-bswap.x" > Did you test this on a thumb1 target? It seems to me that the target parts that you've removed were likely related to that. In fact, I don't see why this test would need to be changed at all. R.
Hi Richard, > Did you test this on a thumb1 target? It seems to me that the target parts that you've > removed were likely related to that. In fact, I don't see why this test would need to be changed at all. The testcase explicitly forces a Thumb-2 target (arm_arch_v6t2). The patterns were wrong for Thumb-2 indeed, and the testcase was explicitly testing for this. There is a separate builtin-bswap-2.c for Thumb-1 target (arm_arch_v6m). Cheers, Wilco
On 26/02/2024 16:05, Wilco Dijkstra wrote: > Hi Richard, > >> Did you test this on a thumb1 target? It seems to me that the target parts that you've >> removed were likely related to that. In fact, I don't see why this test would need to be changed at all. > > The testcase explicitly forces a Thumb-2 target (arm_arch_v6t2). The patterns > were wrong for Thumb-2 indeed, and the testcase was explicitly testing for this. > There is a separate builtin-bswap-2.c for Thumb-1 target (arm_arch_v6m). > > Cheers, > Wilco That's why statements like: * gcc.target/arm/builtin-bswap-1.c: Fix test. are less than helpful. Perhaps if you'd said what you actually changed that would have made it more obvious. So OK, but please fix the commit message to say what you did. R.
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"
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. ---