* [PATCH] ARM: Fix conditional execution [PR113915]
@ 2024-02-21 14:34 Wilco Dijkstra
2024-02-21 15:58 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 6+ messages in thread
From: Wilco Dijkstra @ 2024-02-21 14:34 UTC (permalink / raw)
To: Richard Earnshaw, Kyrylo Tkachov; +Cc: GCC Patches
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"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: Fix conditional execution [PR113915]
2024-02-21 14:34 [PATCH] ARM: Fix conditional execution [PR113915] Wilco Dijkstra
@ 2024-02-21 15:58 ` Richard Earnshaw (lists)
2024-02-23 15:46 ` Wilco Dijkstra
0 siblings, 1 reply; 6+ messages in thread
From: Richard Earnshaw (lists) @ 2024-02-21 15:58 UTC (permalink / raw)
To: Wilco Dijkstra, Kyrylo Tkachov; +Cc: GCC Patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: Fix conditional execution [PR113915]
2024-02-21 15:58 ` Richard Earnshaw (lists)
@ 2024-02-23 15:46 ` Wilco Dijkstra
2024-02-26 15:58 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 6+ messages in thread
From: Wilco Dijkstra @ 2024-02-23 15:46 UTC (permalink / raw)
To: Richard Earnshaw, Kyrylo Tkachov; +Cc: GCC Patches
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"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: Fix conditional execution [PR113915]
2024-02-23 15:46 ` Wilco Dijkstra
@ 2024-02-26 15:58 ` Richard Earnshaw (lists)
2024-02-26 16:05 ` Wilco Dijkstra
0 siblings, 1 reply; 6+ messages in thread
From: Richard Earnshaw (lists) @ 2024-02-26 15:58 UTC (permalink / raw)
To: Wilco Dijkstra, Kyrylo Tkachov; +Cc: GCC Patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: Fix conditional execution [PR113915]
2024-02-26 15:58 ` Richard Earnshaw (lists)
@ 2024-02-26 16:05 ` Wilco Dijkstra
2024-02-26 16:13 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 6+ messages in thread
From: Wilco Dijkstra @ 2024-02-26 16:05 UTC (permalink / raw)
To: Richard Earnshaw, Kyrylo Tkachov; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: Fix conditional execution [PR113915]
2024-02-26 16:05 ` Wilco Dijkstra
@ 2024-02-26 16:13 ` Richard Earnshaw (lists)
0 siblings, 0 replies; 6+ messages in thread
From: Richard Earnshaw (lists) @ 2024-02-26 16:13 UTC (permalink / raw)
To: Wilco Dijkstra, Kyrylo Tkachov; +Cc: GCC Patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-26 16:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-21 14:34 [PATCH] ARM: Fix conditional execution [PR113915] Wilco Dijkstra
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)
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).