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.