[PATCH] arm: Fix ICE with movsi + cmpsi -> movsi_compare0 peephole2 [PR91913]

[PATCH] arm: Fix ICE with movsi + cmpsi -> movsi_compare0 peephole2 [PR91913]

[Jakub Jelinek]

Hi!

The following testcase ICEs on arm.  The problem is that the following
peephole tries to match adjacent *arm_movsi_insn + *arm_cmpsi_insn;
the *arm_movsi_insn uses =rk, rk constraints and *arm_cmpsi_insn
uses r, IL constraints for what is matched by the peephole2.  Peephole2s
work only with predicates and not constraints, and the instruction that
the peephole2 turns those two into is *movsi_compare0, which uses
=r, r constraints.  As operands[1] is the same between the two insns,
sp (k constraint) is not possible for it, but as appears in the testcase,
we try to match sp = non-sp; cc = cmp (non-sp, 0) into
[ cc = cmp (non-sp, 0); sp = non-sp ] insn which isn't then recognized,
because it doesn't match the =r constraint.
I couldn't find a predicate that would do what we need here, the peephole2
uses arm_general_register_operand which matches SUBREGs as well as REGs
and requires <= 15 REGNO or pseudo register.  There is also
arm_hard_general_register_operand predicate that matches only REGs and
requires <= 15 REGNO, so slightly closer to what we need, but nothing
that would require <= LAST_ARM_REGNUM && != SP_REGNUM.

The following patch fixes it just by excluding SP destination in the
peephole2 condition, bootstrapped/regtested on armv7hl-linux-gnueabi,
ok for trunk?

Yet another option would be
s/arm_general_register_operand/arm_hard_general_register_operand/g
in the peephole2 and use
"TARGET_ARM && REGNO (operands[0]) != SP_REGNUM"
as condition.
And yet another option would be to add some predicate that would be
arm_hard_general_register_operand and would require that REGNO != SP_REGNUM
and use it in this peephole2.  But I'd need a suggestion how to call
that predicate, because I have no idea how arm calls core? registers
except sp.

2020-02-01  Jakub Jelinek  <jakub@redhat.com>

	PR target/91913
	* config/arm/arm.md (movsi + cmpsi -> movsi_compare0 peephole2):
	Punt if destination is sp register.

	* gfortran.dg/pr91913.f90: New test.

--- gcc/config/arm/arm.md.jj	2020-01-30 12:57:52.062102735 +0100
+++ gcc/config/arm/arm.md	2020-01-31 15:55:47.199979148 +0100
@@ -11227,7 +11227,7 @@ (define_peephole2
 	(match_operand:SI 1 "arm_general_register_operand" ""))
    (set (reg:CC CC_REGNUM)
 	(compare:CC (match_dup 1) (const_int 0)))]
-  "TARGET_ARM"
+  "TARGET_ARM && REG_P (operands[0]) && REGNO (operands[0]) != SP_REGNUM"
   [(parallel [(set (reg:CC CC_REGNUM) (compare:CC (match_dup 1) (const_int 0)))
 	      (set (match_dup 0) (match_dup 1))])]
   ""
--- gcc/testsuite/gfortran.dg/pr91913.f90.jj	2020-01-31 15:54:06.839475636 +0100
+++ gcc/testsuite/gfortran.dg/pr91913.f90	2020-01-31 15:53:52.265692947 +0100
@@ -0,0 +1,5 @@
+! PR target/91913
+! { dg-do compile }
+! { dg-options "-std=legacy -Ofast --param max-cse-insns=0 -fno-schedule-insns -fsanitize=null" }
+
+include 'string_ctor_1.f90'

	Jakub

Re: [PATCH] arm: Fix ICE with movsi + cmpsi -> movsi_compare0 peephole2 [PR91913]

[Richard Earnshaw (lists)]

On 01/02/2020 08:30, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase ICEs on arm.  The problem is that the following
> peephole tries to match adjacent *arm_movsi_insn + *arm_cmpsi_insn;
> the *arm_movsi_insn uses =rk, rk constraints and *arm_cmpsi_insn
> uses r, IL constraints for what is matched by the peephole2.  Peephole2s
> work only with predicates and not constraints, and the instruction that
> the peephole2 turns those two into is *movsi_compare0, which uses
> =r, r constraints.  As operands[1] is the same between the two insns,
> sp (k constraint) is not possible for it, but as appears in the testcase,
> we try to match sp = non-sp; cc = cmp (non-sp, 0) into
> [ cc = cmp (non-sp, 0); sp = non-sp ] insn which isn't then recognized,
> because it doesn't match the =r constraint.
> I couldn't find a predicate that would do what we need here, the peephole2
> uses arm_general_register_operand which matches SUBREGs as well as REGs
> and requires <= 15 REGNO or pseudo register.  There is also
> arm_hard_general_register_operand predicate that matches only REGs and
> requires <= 15 REGNO, so slightly closer to what we need, but nothing
> that would require <= LAST_ARM_REGNUM && != SP_REGNUM.
> 
> The following patch fixes it just by excluding SP destination in the
> peephole2 condition, bootstrapped/regtested on armv7hl-linux-gnueabi,
> ok for trunk?
> 
> Yet another option would be
> s/arm_general_register_operand/arm_hard_general_register_operand/g
> in the peephole2 and use
> "TARGET_ARM && REGNO (operands[0]) != SP_REGNUM"
> as condition.
> And yet another option would be to add some predicate that would be
> arm_hard_general_register_operand and would require that REGNO != SP_REGNUM
> and use it in this peephole2.  But I'd need a suggestion how to call
> that predicate, because I have no idea how arm calls core? registers
> except sp.

And yet another option would be specify the constraints to 
movsi_compare0 correctly...

The restriction on using SP applies only to Thumb code.  In Arm state SP 
can be used just fine (as was the case when this pattern was originally
written and SP was permitted in constraint 'r'.  When thumb2 support was 
added we split SP into a separate class, but not all patterns were fully 
updated for this.

Since the peephole applies only to Arm state (TARGET_ARM), there's no 
need to change that, but the pattern it hits needs some additional 
variants so that it will match.

I'll commit a patch to do that shortly, but feel free to commit your 
testcase.

Thanks for looking into this though.


R.

(sorry for the delay replying, was in meetings nearly all last week).

> 
> 2020-02-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/91913
> 	* config/arm/arm.md (movsi + cmpsi -> movsi_compare0 peephole2):
> 	Punt if destination is sp register.
> 
> 	* gfortran.dg/pr91913.f90: New test.
> 
> --- gcc/config/arm/arm.md.jj	2020-01-30 12:57:52.062102735 +0100
> +++ gcc/config/arm/arm.md	2020-01-31 15:55:47.199979148 +0100
> @@ -11227,7 +11227,7 @@ (define_peephole2
>   	(match_operand:SI 1 "arm_general_register_operand" ""))
>      (set (reg:CC CC_REGNUM)
>   	(compare:CC (match_dup 1) (const_int 0)))]
> -  "TARGET_ARM"
> +  "TARGET_ARM && REG_P (operands[0]) && REGNO (operands[0]) != SP_REGNUM"
>     [(parallel [(set (reg:CC CC_REGNUM) (compare:CC (match_dup 1) (const_int 0)))
>   	      (set (match_dup 0) (match_dup 1))])]
>     ""
> --- gcc/testsuite/gfortran.dg/pr91913.f90.jj	2020-01-31 15:54:06.839475636 +0100
> +++ gcc/testsuite/gfortran.dg/pr91913.f90	2020-01-31 15:53:52.265692947 +0100
> @@ -0,0 +1,5 @@
> +! PR target/91913
> +! { dg-do compile }
> +! { dg-options "-std=legacy -Ofast --param max-cse-insns=0 -fno-schedule-insns -fsanitize=null" }
> +
> +include 'string_ctor_1.f90'
> 
> 	Jakub
> 

[PATCH] arm: correct constraints on movsi_compare0 [PR91913]

[Richard Earnshaw]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]


The peephole that detects a mov of one register to another followed by
a comparison of the original register against zero is only used in Arm
state; but the instruction that matches this is generic to all 32-bit
compilation states.  That instruction lacks support for SP which is
permitted in Arm state, but has restrictions in Thumb2 code.

This patch fixes the problem by allowing SP when in ARM state for all
registers; in Thumb state it allows SP only as a source when the
register really is copied to another target.

	* config/arm/arm.md (movsi_compare0): Allow SP as a source register
	in Thumb state and also as a destination in Arm state.  Add T16
	variants.
---
 gcc/config/arm/arm.md | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-arm-correct-constraints-on-movsi_compare0-PR91913.patch --]
[-- Type: text/x-patch; name="0001-arm-correct-constraints-on-movsi_compare0-PR91913.patch", Size: 961 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 5baf82d2ad6..ab277996462 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6627,16 +6627,21 @@ (define_expand "builtin_setjmp_receiver"
 
 (define_insn "*movsi_compare0"
   [(set (reg:CC CC_REGNUM)
-	(compare:CC (match_operand:SI 1 "s_register_operand" "0,r")
+	(compare:CC (match_operand:SI 1 "s_register_operand" "0,0,l,rk,rk")
 		    (const_int 0)))
-   (set (match_operand:SI 0 "s_register_operand" "=r,r")
+   (set (match_operand:SI 0 "s_register_operand" "=l,rk,l,r,rk")
 	(match_dup 1))]
   "TARGET_32BIT"
   "@
    cmp%?\\t%0, #0
+   cmp%?\\t%0, #0
+   subs%?\\t%0, %1, #0
+   subs%?\\t%0, %1, #0
    subs%?\\t%0, %1, #0"
   [(set_attr "conds" "set")
-   (set_attr "type" "alus_imm,alus_imm")]
+   (set_attr "arch" "t2,*,t2,t2,a")
+   (set_attr "type" "alus_imm")
+   (set_attr "length" "2,4,2,4,4")]
 )
 
 ;; Subroutine to store a half word from a register into memory.

Re: [PATCH] arm: correct constraints on movsi_compare0 [PR91913]

[Richard Earnshaw]

On 10/02/2020 15:51, Richard Earnshaw wrote:
> 
> The peephole that detects a mov of one register to another followed by
> a comparison of the original register against zero is only used in Arm
> state; but the instruction that matches this is generic to all 32-bit
> compilation states.  That instruction lacks support for SP which is
> permitted in Arm state, but has restrictions in Thumb2 code.
> 
> This patch fixes the problem by allowing SP when in ARM state for all
> registers; in Thumb state it allows SP only as a source when the
> register really is copied to another target.
> 
> 	* config/arm/arm.md (movsi_compare0): Allow SP as a source register
> 	in Thumb state and also as a destination in Arm state.  Add T16
> 	variants.
> ---
>   gcc/config/arm/arm.md | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 

I've now backported this (along with Jakub's test) to gcc-8 and gcc-9.

R.