Re: [PATCH] ARM: Fix conditional execution [PR113915]

[Wilco Dijkstra <Wilco.Dijkstra@arm.com>]

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"