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

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

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.