* [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions
@ 2019-02-18 14:40 Matthew Malcomson
2019-02-22 0:48 ` James Greenhalgh
0 siblings, 1 reply; 5+ messages in thread
From: Matthew Malcomson @ 2019-02-18 14:40 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Earnshaw, James Greenhalgh, nd, Marcus Shawcroft
[-- Attachment #1: Type: text/plain, Size: 18166 bytes --]
Handle stack pointer with SUBS/ADDS instructions.
In general the stack pointer was not handled for many SUBS/ADDS patterns in
aarch64.md.
Both the "extended register" and "immediate" forms allow the stack pointer to be
used as the source register, while no form allows the stack pointer for the
destination register.
The define_insn patterns generating ADDS/SUBS did not allow the stack pointer
for any operand, while the define_peephole2 patterns that generated RTX to be
matched by these patterns allowed the stack pointer for any operand.
The patterns are fixed by adding the 'k' constraint for the first source operand
to all define_insns that generate the ADDS/SUBS "extended register" and
"immediate" forms (but not the "shifted register" form).
In peephole optimizations, constraint strings are ignored (see "(gccint) C
Constraint Interface" info node in the documentation), so the decision to act or
not is based solely on the predicate and condition.
This patch introduces a new predicate "aarch64_general_reg" to be used in
define_peephole2 patterns where only GENERAL_REGS registers are acceptable and
uses that predicate in the peepholes that generate patterns for ADDS/SUBS.
Additionally, this patch contains two tidy-ups (happy to remove them or put in
a separate patch if people want):
We change the condition of sub<mode>3_compare1_imm pattern from checking
"UINTVAL (operands[2]) == -UINTVAL (operands[3])"
to checking
"INTVAL (operands[2]) == -INTVAL (operands[3])"
for clarity, since the values checked are signed integers, there are negations
involved in the check, and the condition used by the corresponding peepholes
also uses INTVAL.
The superfluous <w> iterator in the assembly template for
add<mode>3_compareV_imm is removed -- it was applied to an operand that is
known to be a const_int.
Full bootstrap and regtest done on aarch64-none-linux-gnu.
Regression tests done on aarch64-none-linux-gnu and aarch64-none-elf cross
compiler.
OK for trunk?
NOTE: I have included a bunch of RTL testcases that I used in development, these
don't exercise much of the compiler and are pretty specific to the backend as it
currently is, so I'm not sure they give much value. I'd appreciate feedback on
whether this is in general considered useful.
gcc/ChangeLog:
2019-02-18 Matthew Malcomson <matthew.malcomson@arm.com>
PR target/89324
* config/aarch64/aarch64.md: Use aarch64_general_reg predicate on
destination register in peepholes generating patterns for ADDS/SUBS.
(add<mode>3_compare0,
*addsi3_compare0_uxtw, add<mode>3_compareC,
add<mode>3_compareV_imm, add<mode>3_compareV,
*adds_<optab><ALLX:mode>_<GPI:mode>,
*subs_<optab><ALLX:mode>_<GPI:mode>,
*adds_<optab><ALLX:mode>_shift_<GPI:mode>,
*subs_<optab><ALLX:mode>_shift_<GPI:mode>,
*adds_<optab><mode>_multp2, *subs_<optab><mode>_multp2,
*sub<mode>3_compare0, *subsi3_compare0_uxtw,
sub<mode>3_compare1): Allow stack pointer for source register.
* config/aarch64/predicates.md (aarch64_general_reg): New predicate.
gcc/testsuite/ChangeLog:
2019-02-18 Matthew Malcomson <matthew.malcomson@arm.com>
PR target/89324
* gcc.dg/rtl/aarch64/subs_adds_sp.c: New test.
* gfortran.fortran-torture/compile/pr89324.f90: New test.
############### Attachment also inlined for ease of reply ###############
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index b7f6fe0f1354f7aa19076a946ed2c633b9b9b8da..0d5754a21e31b0c53afb320bdf574fa4a43c7573 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1985,7 +1985,7 @@ (define_expand "uaddvti4"
(define_insn "add<mode>3_compare0"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (plus:GPI (match_operand:GPI 1 "register_operand" "%r,r,r")
+ (plus:GPI (match_operand:GPI 1 "register_operand" "%rk,rk,rk")
(match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r,r,r")
@@ -2002,7 +2002,7 @@ (define_insn "add<mode>3_compare0"
(define_insn "*addsi3_compare0_uxtw"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (plus:SI (match_operand:SI 1 "register_operand" "%r,r,r")
+ (plus:SI (match_operand:SI 1 "register_operand" "%rk,rk,rk")
(match_operand:SI 2 "aarch64_plus_operand" "r,I,J"))
(const_int 0)))
(set (match_operand:DI 0 "register_operand" "=r,r,r")
@@ -2034,7 +2034,7 @@ (define_insn "add<mode>3_compareC"
[(set (reg:CC_C CC_REGNUM)
(compare:CC_C
(plus:GPI
- (match_operand:GPI 1 "register_operand" "r,r,r")
+ (match_operand:GPI 1 "register_operand" "rk,rk,rk")
(match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))
(match_dup 1)))
(set (match_operand:GPI 0 "register_operand" "=r,r,r")
@@ -2081,7 +2081,7 @@ (define_insn "add<mode>3_compareV_imm"
(compare:CC_V
(plus:<DWI>
(sign_extend:<DWI>
- (match_operand:GPI 1 "register_operand" "r,r"))
+ (match_operand:GPI 1 "register_operand" "rk,rk"))
(match_operand:GPI 2 "aarch64_plus_immediate" "I,J"))
(sign_extend:<DWI>
(plus:GPI (match_dup 1) (match_dup 2)))))
@@ -2089,7 +2089,7 @@ (define_insn "add<mode>3_compareV_imm"
(plus:GPI (match_dup 1) (match_dup 2)))]
""
"@
- adds\\t%<w>0, %<w>1, %<w>2
+ adds\\t%<w>0, %<w>1, %2
subs\\t%<w>0, %<w>1, #%n2"
[(set_attr "type" "alus_imm,alus_imm")]
)
@@ -2098,7 +2098,7 @@ (define_insn "add<mode>3_compareV"
[(set (reg:CC_V CC_REGNUM)
(compare:CC_V
(plus:<DWI>
- (sign_extend:<DWI> (match_operand:GPI 1 "register_operand" "r"))
+ (sign_extend:<DWI> (match_operand:GPI 1 "register_operand" "rk"))
(sign_extend:<DWI> (match_operand:GPI 2 "register_operand" "r")))
(sign_extend:<DWI> (plus:GPI (match_dup 1) (match_dup 2)))))
(set (match_operand:GPI 0 "register_operand" "=r")
@@ -2177,7 +2177,7 @@ (define_insn "*adds_<optab><ALLX:mode>_<GPI:mode>"
(compare:CC_NZ
(plus:GPI
(ANY_EXTEND:GPI (match_operand:ALLX 1 "register_operand" "r"))
- (match_operand:GPI 2 "register_operand" "r"))
+ (match_operand:GPI 2 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ANY_EXTEND:GPI (match_dup 1)) (match_dup 2)))]
@@ -2189,7 +2189,7 @@ (define_insn "*adds_<optab><ALLX:mode>_<GPI:mode>"
(define_insn "*subs_<optab><ALLX:mode>_<GPI:mode>"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(ANY_EXTEND:GPI
(match_operand:ALLX 2 "register_operand" "r")))
(const_int 0)))
@@ -2207,7 +2207,7 @@ (define_insn "*adds_<optab><ALLX:mode>_shift_<GPI:mode>"
(ANY_EXTEND:GPI
(match_operand:ALLX 1 "register_operand" "r"))
(match_operand 2 "aarch64_imm3" "Ui3"))
- (match_operand:GPI 3 "register_operand" "r"))
+ (match_operand:GPI 3 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=rk")
(plus:GPI (ashift:GPI (ANY_EXTEND:GPI (match_dup 1))
@@ -2221,7 +2221,7 @@ (define_insn "*adds_<optab><ALLX:mode>_shift_<GPI:mode>"
(define_insn "*subs_<optab><ALLX:mode>_shift_<GPI:mode>"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(ashift:GPI
(ANY_EXTEND:GPI
(match_operand:ALLX 2 "register_operand" "r"))
@@ -2244,7 +2244,7 @@ (define_insn "*adds_<optab><mode>_multp2"
(match_operand 2 "aarch64_pwr_imm3" "Up3"))
(match_operand 3 "const_int_operand" "n")
(const_int 0))
- (match_operand:GPI 4 "register_operand" "r"))
+ (match_operand:GPI 4 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ANY_EXTRACT:GPI (mult:GPI (match_dup 1) (match_dup 2))
@@ -2259,7 +2259,7 @@ (define_insn "*adds_<optab><mode>_multp2"
(define_insn "*subs_<optab><mode>_multp2"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 4 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 4 "register_operand" "rk")
(ANY_EXTRACT:GPI
(mult:GPI (match_operand:GPI 1 "register_operand" "r")
(match_operand 2 "aarch64_pwr_imm3" "Up3"))
@@ -2923,7 +2923,7 @@ (define_insn "negvdi_carryinV"
(define_insn "*sub<mode>3_compare0"
[(set (reg:CC_NZ CC_REGNUM)
- (compare:CC_NZ (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (compare:CC_NZ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(match_operand:GPI 2 "register_operand" "r"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
@@ -2936,7 +2936,7 @@ (define_insn "*sub<mode>3_compare0"
;; zero_extend version of above
(define_insn "*subsi3_compare0_uxtw"
[(set (reg:CC_NZ CC_REGNUM)
- (compare:CC_NZ (minus:SI (match_operand:SI 1 "register_operand" "r")
+ (compare:CC_NZ (minus:SI (match_operand:SI 1 "register_operand" "rk")
(match_operand:SI 2 "register_operand" "r"))
(const_int 0)))
(set (match_operand:DI 0 "register_operand" "=r")
@@ -2949,13 +2949,13 @@ (define_insn "*subsi3_compare0_uxtw"
(define_insn "sub<mode>3_compare1_imm"
[(set (reg:CC CC_REGNUM)
(compare:CC
- (match_operand:GPI 1 "aarch64_reg_or_zero" "rZ,rZ")
+ (match_operand:GPI 1 "aarch64_reg_or_zero" "rkZ,rkZ")
(match_operand:GPI 2 "aarch64_plus_immediate" "I,J")))
(set (match_operand:GPI 0 "register_operand" "=r,r")
(plus:GPI
(match_dup 1)
(match_operand:GPI 3 "aarch64_plus_immediate" "J,I")))]
- "UINTVAL (operands[2]) == -UINTVAL (operands[3])"
+ "INTVAL (operands[2]) == -INTVAL (operands[3])"
"@
subs\\t%<w>0, %<w>1, %2
adds\\t%<w>0, %<w>1, #%n2"
@@ -2965,7 +2965,7 @@ (define_insn "sub<mode>3_compare1_imm"
(define_insn "sub<mode>3_compare1"
[(set (reg:CC CC_REGNUM)
(compare:CC
- (match_operand:GPI 1 "aarch64_reg_or_zero" "rZ")
+ (match_operand:GPI 1 "aarch64_reg_or_zero" "rkZ")
(match_operand:GPI 2 "aarch64_reg_or_zero" "rZ")))
(set (match_operand:GPI 0 "register_operand" "=r")
(minus:GPI (match_dup 1) (match_dup 2)))]
@@ -2975,7 +2975,7 @@ (define_insn "sub<mode>3_compare1"
)
(define_peephole2
- [(set (match_operand:GPI 0 "register_operand")
+ [(set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_operand:GPI 1 "aarch64_reg_or_zero")
(match_operand:GPI 2 "aarch64_reg_or_zero")))
(set (reg:CC CC_REGNUM)
@@ -3000,7 +3000,7 @@ (define_peephole2
(compare:CC
(match_operand:GPI 1 "aarch64_reg_or_zero")
(match_operand:GPI 2 "aarch64_reg_or_zero")))
- (set (match_operand:GPI 0 "register_operand")
+ (set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_dup 1)
(match_dup 2)))]
""
@@ -3013,7 +3013,7 @@ (define_peephole2
)
(define_peephole2
- [(set (match_operand:GPI 0 "register_operand")
+ [(set (match_operand:GPI 0 "aarch64_general_reg")
(plus:GPI (match_operand:GPI 1 "register_operand")
(match_operand:GPI 2 "aarch64_plus_immediate")))
(set (reg:CC CC_REGNUM)
@@ -3038,7 +3038,7 @@ (define_peephole2
(compare:CC
(match_operand:GPI 1 "register_operand")
(match_operand:GPI 3 "const_int_operand")))
- (set (match_operand:GPI 0 "register_operand")
+ (set (match_operand:GPI 0 "aarch64_general_reg")
(plus:GPI (match_dup 1)
(match_operand:GPI 2 "aarch64_plus_immediate")))]
"INTVAL (operands[3]) == -INTVAL (operands[2])"
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index b8e6d232ff6237a58addda1ec0e953a1054dc616..8e1b784217b0367dc647a87024e14e36de781008 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -30,6 +30,10 @@ (define_predicate "aarch64_call_insn_operand"
(ior (match_code "symbol_ref")
(match_operand 0 "register_operand")))
+(define_predicate "aarch64_general_reg"
+ (and (match_operand 0 "register_operand")
+ (match_test "REGNO_REG_CLASS (REGNO (op)) == GENERAL_REGS")))
+
;; Return true if OP a (const_int 0) operand.
(define_predicate "const0_operand"
(and (match_code "const_int")
diff --git a/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c b/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c
new file mode 100644
index 0000000000000000000000000000000000000000..f537771a271e1d33df29561a4b687d621e090bab
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c
@@ -0,0 +1,153 @@
+/* { dg-do compile { target aarch64-*-* } } */
+/* { dg-options "-O2" } */
+/*
+ Tests are:
+ Patterns allow subs/adds with a stack pointer source.
+ define_peephole2's don't generate patterns for subs/adds with a stack
+ pointer destination.
+ */
+
+/* These functions used to ICE due to using the stack pointer as a source
+ register. */
+
+int __RTL (startwith ("final"))
+adds ()
+{
+(function "adds"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 101 (parallel [
+ (set (reg:CC cc)
+ (compare:CC (reg/f:DI sp)
+ (const_int -3)))
+ (set (reg/f:DI x19)
+ (plus:DI (reg/f:DI sp)
+ (const_int 3)))
+ ]))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 10 (use (reg/i:SI x19)))
+ (cinsn 11 (use (reg/i:SI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("final"))
+subs ()
+{
+(function "subs"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 101 (parallel [
+ (set (reg:CC cc)
+ (compare:CC (reg/f:DI sp)
+ (const_int 3)))
+ (set (reg/f:DI x19)
+ (plus:DI (reg/f:DI sp)
+ (const_int -3)))
+ ]))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 10 (use (reg/i:SI x19)))
+ (cinsn 11 (use (reg/i:SI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+/* These functions used to trigger peepholes generating invalid SUBS patterns
+ that used the stack pointer for the destination register. */
+
+int __RTL (startwith ("peephole2")) sub3_compare1_peephole_1 ()
+{
+(function "sub3_compare1_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_peephole_2 ()
+{
+(function "sub3_compare1_peephole_2"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_imm_peephole_1 ()
+{
+(function "sub3_compare1_imm_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_imm_peephole_2 ()
+{
+(function "sub3_compare1_imm_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+/* Verify that the adds and subs functions generated their respective
+ instructions, and that none of the other functions generated either since
+ they are setting the stack pointer. */
+/* { dg-final { scan-assembler-times {adds\tx[0-9]+, sp} 1 } } */
+/* { dg-final { scan-assembler-not {adds\tsp} } } */
+/* { dg-final { scan-assembler-times {subs\tx[0-9]+, sp} 1 } } */
+/* { dg-final { scan-assembler-not {subs\tsp} } } */
+
diff --git a/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90 b/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90
new file mode 100644
index 0000000000000000000000000000000000000000..014b655d5edae26689ff57eda808e2a0f6146d5b
--- /dev/null
+++ b/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90
@@ -0,0 +1,15 @@
+module a
+contains
+ pure function myotherlen()
+ myotherlen = 99
+ end
+ subroutine b
+ characterx
+ block
+ character(myotherlen()) c
+ c = "abc"
+ x = c
+ end block
+
+ end
+end
[-- Attachment #2: subs-adds-handle-sp.patch.patch --]
[-- Type: text/plain, Size: 14379 bytes --]
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index b7f6fe0f1354f7aa19076a946ed2c633b9b9b8da..0d5754a21e31b0c53afb320bdf574fa4a43c7573 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1985,7 +1985,7 @@ (define_expand "uaddvti4"
(define_insn "add<mode>3_compare0"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (plus:GPI (match_operand:GPI 1 "register_operand" "%r,r,r")
+ (plus:GPI (match_operand:GPI 1 "register_operand" "%rk,rk,rk")
(match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r,r,r")
@@ -2002,7 +2002,7 @@ (define_insn "add<mode>3_compare0"
(define_insn "*addsi3_compare0_uxtw"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (plus:SI (match_operand:SI 1 "register_operand" "%r,r,r")
+ (plus:SI (match_operand:SI 1 "register_operand" "%rk,rk,rk")
(match_operand:SI 2 "aarch64_plus_operand" "r,I,J"))
(const_int 0)))
(set (match_operand:DI 0 "register_operand" "=r,r,r")
@@ -2034,7 +2034,7 @@ (define_insn "add<mode>3_compareC"
[(set (reg:CC_C CC_REGNUM)
(compare:CC_C
(plus:GPI
- (match_operand:GPI 1 "register_operand" "r,r,r")
+ (match_operand:GPI 1 "register_operand" "rk,rk,rk")
(match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))
(match_dup 1)))
(set (match_operand:GPI 0 "register_operand" "=r,r,r")
@@ -2081,7 +2081,7 @@ (define_insn "add<mode>3_compareV_imm"
(compare:CC_V
(plus:<DWI>
(sign_extend:<DWI>
- (match_operand:GPI 1 "register_operand" "r,r"))
+ (match_operand:GPI 1 "register_operand" "rk,rk"))
(match_operand:GPI 2 "aarch64_plus_immediate" "I,J"))
(sign_extend:<DWI>
(plus:GPI (match_dup 1) (match_dup 2)))))
@@ -2089,7 +2089,7 @@ (define_insn "add<mode>3_compareV_imm"
(plus:GPI (match_dup 1) (match_dup 2)))]
""
"@
- adds\\t%<w>0, %<w>1, %<w>2
+ adds\\t%<w>0, %<w>1, %2
subs\\t%<w>0, %<w>1, #%n2"
[(set_attr "type" "alus_imm,alus_imm")]
)
@@ -2098,7 +2098,7 @@ (define_insn "add<mode>3_compareV"
[(set (reg:CC_V CC_REGNUM)
(compare:CC_V
(plus:<DWI>
- (sign_extend:<DWI> (match_operand:GPI 1 "register_operand" "r"))
+ (sign_extend:<DWI> (match_operand:GPI 1 "register_operand" "rk"))
(sign_extend:<DWI> (match_operand:GPI 2 "register_operand" "r")))
(sign_extend:<DWI> (plus:GPI (match_dup 1) (match_dup 2)))))
(set (match_operand:GPI 0 "register_operand" "=r")
@@ -2177,7 +2177,7 @@ (define_insn "*adds_<optab><ALLX:mode>_<GPI:mode>"
(compare:CC_NZ
(plus:GPI
(ANY_EXTEND:GPI (match_operand:ALLX 1 "register_operand" "r"))
- (match_operand:GPI 2 "register_operand" "r"))
+ (match_operand:GPI 2 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ANY_EXTEND:GPI (match_dup 1)) (match_dup 2)))]
@@ -2189,7 +2189,7 @@ (define_insn "*adds_<optab><ALLX:mode>_<GPI:mode>"
(define_insn "*subs_<optab><ALLX:mode>_<GPI:mode>"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(ANY_EXTEND:GPI
(match_operand:ALLX 2 "register_operand" "r")))
(const_int 0)))
@@ -2207,7 +2207,7 @@ (define_insn "*adds_<optab><ALLX:mode>_shift_<GPI:mode>"
(ANY_EXTEND:GPI
(match_operand:ALLX 1 "register_operand" "r"))
(match_operand 2 "aarch64_imm3" "Ui3"))
- (match_operand:GPI 3 "register_operand" "r"))
+ (match_operand:GPI 3 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=rk")
(plus:GPI (ashift:GPI (ANY_EXTEND:GPI (match_dup 1))
@@ -2221,7 +2221,7 @@ (define_insn "*adds_<optab><ALLX:mode>_shift_<GPI:mode>"
(define_insn "*subs_<optab><ALLX:mode>_shift_<GPI:mode>"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(ashift:GPI
(ANY_EXTEND:GPI
(match_operand:ALLX 2 "register_operand" "r"))
@@ -2244,7 +2244,7 @@ (define_insn "*adds_<optab><mode>_multp2"
(match_operand 2 "aarch64_pwr_imm3" "Up3"))
(match_operand 3 "const_int_operand" "n")
(const_int 0))
- (match_operand:GPI 4 "register_operand" "r"))
+ (match_operand:GPI 4 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ANY_EXTRACT:GPI (mult:GPI (match_dup 1) (match_dup 2))
@@ -2259,7 +2259,7 @@ (define_insn "*adds_<optab><mode>_multp2"
(define_insn "*subs_<optab><mode>_multp2"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 4 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 4 "register_operand" "rk")
(ANY_EXTRACT:GPI
(mult:GPI (match_operand:GPI 1 "register_operand" "r")
(match_operand 2 "aarch64_pwr_imm3" "Up3"))
@@ -2923,7 +2923,7 @@ (define_insn "negvdi_carryinV"
(define_insn "*sub<mode>3_compare0"
[(set (reg:CC_NZ CC_REGNUM)
- (compare:CC_NZ (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (compare:CC_NZ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(match_operand:GPI 2 "register_operand" "r"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
@@ -2936,7 +2936,7 @@ (define_insn "*sub<mode>3_compare0"
;; zero_extend version of above
(define_insn "*subsi3_compare0_uxtw"
[(set (reg:CC_NZ CC_REGNUM)
- (compare:CC_NZ (minus:SI (match_operand:SI 1 "register_operand" "r")
+ (compare:CC_NZ (minus:SI (match_operand:SI 1 "register_operand" "rk")
(match_operand:SI 2 "register_operand" "r"))
(const_int 0)))
(set (match_operand:DI 0 "register_operand" "=r")
@@ -2949,13 +2949,13 @@ (define_insn "*subsi3_compare0_uxtw"
(define_insn "sub<mode>3_compare1_imm"
[(set (reg:CC CC_REGNUM)
(compare:CC
- (match_operand:GPI 1 "aarch64_reg_or_zero" "rZ,rZ")
+ (match_operand:GPI 1 "aarch64_reg_or_zero" "rkZ,rkZ")
(match_operand:GPI 2 "aarch64_plus_immediate" "I,J")))
(set (match_operand:GPI 0 "register_operand" "=r,r")
(plus:GPI
(match_dup 1)
(match_operand:GPI 3 "aarch64_plus_immediate" "J,I")))]
- "UINTVAL (operands[2]) == -UINTVAL (operands[3])"
+ "INTVAL (operands[2]) == -INTVAL (operands[3])"
"@
subs\\t%<w>0, %<w>1, %2
adds\\t%<w>0, %<w>1, #%n2"
@@ -2965,7 +2965,7 @@ (define_insn "sub<mode>3_compare1_imm"
(define_insn "sub<mode>3_compare1"
[(set (reg:CC CC_REGNUM)
(compare:CC
- (match_operand:GPI 1 "aarch64_reg_or_zero" "rZ")
+ (match_operand:GPI 1 "aarch64_reg_or_zero" "rkZ")
(match_operand:GPI 2 "aarch64_reg_or_zero" "rZ")))
(set (match_operand:GPI 0 "register_operand" "=r")
(minus:GPI (match_dup 1) (match_dup 2)))]
@@ -2975,7 +2975,7 @@ (define_insn "sub<mode>3_compare1"
)
(define_peephole2
- [(set (match_operand:GPI 0 "register_operand")
+ [(set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_operand:GPI 1 "aarch64_reg_or_zero")
(match_operand:GPI 2 "aarch64_reg_or_zero")))
(set (reg:CC CC_REGNUM)
@@ -3000,7 +3000,7 @@ (define_peephole2
(compare:CC
(match_operand:GPI 1 "aarch64_reg_or_zero")
(match_operand:GPI 2 "aarch64_reg_or_zero")))
- (set (match_operand:GPI 0 "register_operand")
+ (set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_dup 1)
(match_dup 2)))]
""
@@ -3013,7 +3013,7 @@ (define_peephole2
)
(define_peephole2
- [(set (match_operand:GPI 0 "register_operand")
+ [(set (match_operand:GPI 0 "aarch64_general_reg")
(plus:GPI (match_operand:GPI 1 "register_operand")
(match_operand:GPI 2 "aarch64_plus_immediate")))
(set (reg:CC CC_REGNUM)
@@ -3038,7 +3038,7 @@ (define_peephole2
(compare:CC
(match_operand:GPI 1 "register_operand")
(match_operand:GPI 3 "const_int_operand")))
- (set (match_operand:GPI 0 "register_operand")
+ (set (match_operand:GPI 0 "aarch64_general_reg")
(plus:GPI (match_dup 1)
(match_operand:GPI 2 "aarch64_plus_immediate")))]
"INTVAL (operands[3]) == -INTVAL (operands[2])"
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index b8e6d232ff6237a58addda1ec0e953a1054dc616..8e1b784217b0367dc647a87024e14e36de781008 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -30,6 +30,10 @@ (define_predicate "aarch64_call_insn_operand"
(ior (match_code "symbol_ref")
(match_operand 0 "register_operand")))
+(define_predicate "aarch64_general_reg"
+ (and (match_operand 0 "register_operand")
+ (match_test "REGNO_REG_CLASS (REGNO (op)) == GENERAL_REGS")))
+
;; Return true if OP a (const_int 0) operand.
(define_predicate "const0_operand"
(and (match_code "const_int")
diff --git a/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c b/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c
new file mode 100644
index 0000000000000000000000000000000000000000..f537771a271e1d33df29561a4b687d621e090bab
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c
@@ -0,0 +1,153 @@
+/* { dg-do compile { target aarch64-*-* } } */
+/* { dg-options "-O2" } */
+/*
+ Tests are:
+ Patterns allow subs/adds with a stack pointer source.
+ define_peephole2's don't generate patterns for subs/adds with a stack
+ pointer destination.
+ */
+
+/* These functions used to ICE due to using the stack pointer as a source
+ register. */
+
+int __RTL (startwith ("final"))
+adds ()
+{
+(function "adds"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 101 (parallel [
+ (set (reg:CC cc)
+ (compare:CC (reg/f:DI sp)
+ (const_int -3)))
+ (set (reg/f:DI x19)
+ (plus:DI (reg/f:DI sp)
+ (const_int 3)))
+ ]))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 10 (use (reg/i:SI x19)))
+ (cinsn 11 (use (reg/i:SI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("final"))
+subs ()
+{
+(function "subs"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 101 (parallel [
+ (set (reg:CC cc)
+ (compare:CC (reg/f:DI sp)
+ (const_int 3)))
+ (set (reg/f:DI x19)
+ (plus:DI (reg/f:DI sp)
+ (const_int -3)))
+ ]))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 10 (use (reg/i:SI x19)))
+ (cinsn 11 (use (reg/i:SI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+/* These functions used to trigger peepholes generating invalid SUBS patterns
+ that used the stack pointer for the destination register. */
+
+int __RTL (startwith ("peephole2")) sub3_compare1_peephole_1 ()
+{
+(function "sub3_compare1_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_peephole_2 ()
+{
+(function "sub3_compare1_peephole_2"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_imm_peephole_1 ()
+{
+(function "sub3_compare1_imm_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_imm_peephole_2 ()
+{
+(function "sub3_compare1_imm_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+/* Verify that the adds and subs functions generated their respective
+ instructions, and that none of the other functions generated either since
+ they are setting the stack pointer. */
+/* { dg-final { scan-assembler-times {adds\tx[0-9]+, sp} 1 } } */
+/* { dg-final { scan-assembler-not {adds\tsp} } } */
+/* { dg-final { scan-assembler-times {subs\tx[0-9]+, sp} 1 } } */
+/* { dg-final { scan-assembler-not {subs\tsp} } } */
+
diff --git a/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90 b/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90
new file mode 100644
index 0000000000000000000000000000000000000000..014b655d5edae26689ff57eda808e2a0f6146d5b
--- /dev/null
+++ b/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90
@@ -0,0 +1,15 @@
+module a
+contains
+ pure function myotherlen()
+ myotherlen = 99
+ end
+ subroutine b
+ characterx
+ block
+ character(myotherlen()) c
+ c = "abc"
+ x = c
+ end block
+
+ end
+end
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions
2019-02-18 14:40 [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions Matthew Malcomson
@ 2019-02-22 0:48 ` James Greenhalgh
2019-02-22 6:47 ` Mike Stump
2019-02-22 16:11 ` Matthew Malcomson
0 siblings, 2 replies; 5+ messages in thread
From: James Greenhalgh @ 2019-02-22 0:48 UTC (permalink / raw)
To: Matthew Malcomson; +Cc: gcc-patches, Richard Earnshaw, nd, Marcus Shawcroft
On Mon, Feb 18, 2019 at 08:40:12AM -0600, Matthew Malcomson wrote:
> Handle stack pointer with SUBS/ADDS instructions.
>
> In general the stack pointer was not handled for many SUBS/ADDS patterns in
> aarch64.md.
> Both the "extended register" and "immediate" forms allow the stack pointer to be
> used as the source register, while no form allows the stack pointer for the
> destination register.
>
> The define_insn patterns generating ADDS/SUBS did not allow the stack pointer
> for any operand, while the define_peephole2 patterns that generated RTX to be
> matched by these patterns allowed the stack pointer for any operand.
>
> The patterns are fixed by adding the 'k' constraint for the first source operand
> to all define_insns that generate the ADDS/SUBS "extended register" and
> "immediate" forms (but not the "shifted register" form).
>
> In peephole optimizations, constraint strings are ignored (see "(gccint) C
> Constraint Interface" info node in the documentation), so the decision to act or
> not is based solely on the predicate and condition.
> This patch introduces a new predicate "aarch64_general_reg" to be used in
> define_peephole2 patterns where only GENERAL_REGS registers are acceptable and
> uses that predicate in the peepholes that generate patterns for ADDS/SUBS.
>
> Additionally, this patch contains two tidy-ups (happy to remove them or put in
> a separate patch if people want):
Generally, yes I prefer that.
> We change the condition of sub<mode>3_compare1_imm pattern from checking
> "UINTVAL (operands[2]) == -UINTVAL (operands[3])"
> to checking
> "INTVAL (operands[2]) == -INTVAL (operands[3])"
> for clarity, since the values checked are signed integers, there are negations
> involved in the check, and the condition used by the corresponding peepholes
> also uses INTVAL.
>
> The superfluous <w> iterator in the assembly template for
> add<mode>3_compareV_imm is removed -- it was applied to an operand that is
> known to be a const_int.
>
> Full bootstrap and regtest done on aarch64-none-linux-gnu.
> Regression tests done on aarch64-none-linux-gnu and aarch64-none-elf cross
> compiler.
>
> OK for trunk?
This patch is fine for GCC 10, so not on trunk yet please unless there is
a corectness reason for the change.
> NOTE: I have included a bunch of RTL testcases that I used in development, these
> don't exercise much of the compiler and are pretty specific to the backend as it
> currently is, so I'm not sure they give much value. I'd appreciate feedback on
> whether this is in general considered useful.
I would happily take the RTL test, do you want to check with a testsuite
maintainer?
> gcc/ChangeLog:
>
> 2019-02-18 Matthew Malcomson <matthew.malcomson@arm.com>
>
> PR target/89324
> * config/aarch64/aarch64.md: Use aarch64_general_reg predicate on
> destination register in peepholes generating patterns for ADDS/SUBS.
> (add<mode>3_compare0,
> *addsi3_compare0_uxtw, add<mode>3_compareC,
> add<mode>3_compareV_imm, add<mode>3_compareV,
> *adds_<optab><ALLX:mode>_<GPI:mode>,
> *subs_<optab><ALLX:mode>_<GPI:mode>,
> *adds_<optab><ALLX:mode>_shift_<GPI:mode>,
> *subs_<optab><ALLX:mode>_shift_<GPI:mode>,
> *adds_<optab><mode>_multp2, *subs_<optab><mode>_multp2,
> *sub<mode>3_compare0, *subsi3_compare0_uxtw,
> sub<mode>3_compare1): Allow stack pointer for source register.
> * config/aarch64/predicates.md (aarch64_general_reg): New predicate.
>
>
> gcc/testsuite/ChangeLog:
>
> 2019-02-18 Matthew Malcomson <matthew.malcomson@arm.com>
>
> PR target/89324
> * gcc.dg/rtl/aarch64/subs_adds_sp.c: New test.
> * gfortran.fortran-torture/compile/pr89324.f90: New test.
>
>
>
> ############### Attachment also inlined for ease of reply ###############
I appreciate that.
Thanks,
James
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions
2019-02-22 0:48 ` James Greenhalgh
@ 2019-02-22 6:47 ` Mike Stump
2019-02-22 16:11 ` Matthew Malcomson
1 sibling, 0 replies; 5+ messages in thread
From: Mike Stump @ 2019-02-22 6:47 UTC (permalink / raw)
To: James Greenhalgh
Cc: Matthew Malcomson, gcc-patches, Richard Earnshaw, nd, Marcus Shawcroft
On Feb 21, 2019, at 4:09 PM, James Greenhalgh <james.greenhalgh@arm.com> wrote:
>
>> NOTE: I have included a bunch of RTL testcases that I used in development, these
>> don't exercise much of the compiler and are pretty specific to the backend as it
>> currently is, so I'm not sure they give much value. I'd appreciate feedback on
>> whether this is in general considered useful.
>
> I would happily take the RTL test, do you want to check with a testsuite
> maintainer?
If someone asked me, I'd leave it up to the maintainer that is tasked with maintaining the test case. If too much trouble and a ton of maintenance required for not much benefit, they might prefer not to have it. Anyway, I'd defer to the port people to weigh in. Doesn't look unreasonable at first blush.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions
2019-02-22 0:48 ` James Greenhalgh
2019-02-22 6:47 ` Mike Stump
@ 2019-02-22 16:11 ` Matthew Malcomson
2019-02-22 16:25 ` James Greenhalgh
1 sibling, 1 reply; 5+ messages in thread
From: Matthew Malcomson @ 2019-02-22 16:11 UTC (permalink / raw)
To: James Greenhalgh; +Cc: gcc-patches, Richard Earnshaw, nd, Marcus Shawcroft
[-- Attachment #1: Type: text/plain, Size: 761 bytes --]
Hi James,
On 22/02/19 00:09, James Greenhalgh wrote:
> On Mon, Feb 18, 2019 at 08:40:12AM -0600, Matthew Malcomson wrote:
>>
>> Additionally, this patch contains two tidy-ups (happy to remove them or put in
>> a separate patch if people want):
>
> Generally, yes I prefer that.
>
I've removed the tidy ups and retested -- modified patch attached.
>>
>> OK for trunk?
>
> This patch is fine for GCC 10, so not on trunk yet please unless there is
> a corectness reason for the change.
>
The patch is a correctness change to fix the P1 regression that occurs
when the stack pointer was passed to the define_insn from the
define_peephole2.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89324
Ok for trunk?
Regards,
Matthew
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: subs-changed.patch --]
[-- Type: text/x-patch; name="subs-changed.patch", Size: 14249 bytes --]
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index b7f6fe0f1354f7aa19076a946ed2c633b9b9b8da..b7cd9fc787b8526cc7b11b9430fbfcd73103328a 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1985,7 +1985,7 @@ (define_expand "uaddvti4"
(define_insn "add<mode>3_compare0"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (plus:GPI (match_operand:GPI 1 "register_operand" "%r,r,r")
+ (plus:GPI (match_operand:GPI 1 "register_operand" "%rk,rk,rk")
(match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r,r,r")
@@ -2002,7 +2002,7 @@ (define_insn "add<mode>3_compare0"
(define_insn "*addsi3_compare0_uxtw"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (plus:SI (match_operand:SI 1 "register_operand" "%r,r,r")
+ (plus:SI (match_operand:SI 1 "register_operand" "%rk,rk,rk")
(match_operand:SI 2 "aarch64_plus_operand" "r,I,J"))
(const_int 0)))
(set (match_operand:DI 0 "register_operand" "=r,r,r")
@@ -2034,7 +2034,7 @@ (define_insn "add<mode>3_compareC"
[(set (reg:CC_C CC_REGNUM)
(compare:CC_C
(plus:GPI
- (match_operand:GPI 1 "register_operand" "r,r,r")
+ (match_operand:GPI 1 "register_operand" "rk,rk,rk")
(match_operand:GPI 2 "aarch64_plus_operand" "r,I,J"))
(match_dup 1)))
(set (match_operand:GPI 0 "register_operand" "=r,r,r")
@@ -2081,7 +2081,7 @@ (define_insn "add<mode>3_compareV_imm"
(compare:CC_V
(plus:<DWI>
(sign_extend:<DWI>
- (match_operand:GPI 1 "register_operand" "r,r"))
+ (match_operand:GPI 1 "register_operand" "rk,rk"))
(match_operand:GPI 2 "aarch64_plus_immediate" "I,J"))
(sign_extend:<DWI>
(plus:GPI (match_dup 1) (match_dup 2)))))
@@ -2098,7 +2098,7 @@ (define_insn "add<mode>3_compareV"
[(set (reg:CC_V CC_REGNUM)
(compare:CC_V
(plus:<DWI>
- (sign_extend:<DWI> (match_operand:GPI 1 "register_operand" "r"))
+ (sign_extend:<DWI> (match_operand:GPI 1 "register_operand" "rk"))
(sign_extend:<DWI> (match_operand:GPI 2 "register_operand" "r")))
(sign_extend:<DWI> (plus:GPI (match_dup 1) (match_dup 2)))))
(set (match_operand:GPI 0 "register_operand" "=r")
@@ -2177,7 +2177,7 @@ (define_insn "*adds_<optab><ALLX:mode>_<GPI:mode>"
(compare:CC_NZ
(plus:GPI
(ANY_EXTEND:GPI (match_operand:ALLX 1 "register_operand" "r"))
- (match_operand:GPI 2 "register_operand" "r"))
+ (match_operand:GPI 2 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ANY_EXTEND:GPI (match_dup 1)) (match_dup 2)))]
@@ -2189,7 +2189,7 @@ (define_insn "*adds_<optab><ALLX:mode>_<GPI:mode>"
(define_insn "*subs_<optab><ALLX:mode>_<GPI:mode>"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(ANY_EXTEND:GPI
(match_operand:ALLX 2 "register_operand" "r")))
(const_int 0)))
@@ -2207,7 +2207,7 @@ (define_insn "*adds_<optab><ALLX:mode>_shift_<GPI:mode>"
(ANY_EXTEND:GPI
(match_operand:ALLX 1 "register_operand" "r"))
(match_operand 2 "aarch64_imm3" "Ui3"))
- (match_operand:GPI 3 "register_operand" "r"))
+ (match_operand:GPI 3 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=rk")
(plus:GPI (ashift:GPI (ANY_EXTEND:GPI (match_dup 1))
@@ -2221,7 +2221,7 @@ (define_insn "*adds_<optab><ALLX:mode>_shift_<GPI:mode>"
(define_insn "*subs_<optab><ALLX:mode>_shift_<GPI:mode>"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(ashift:GPI
(ANY_EXTEND:GPI
(match_operand:ALLX 2 "register_operand" "r"))
@@ -2244,7 +2244,7 @@ (define_insn "*adds_<optab><mode>_multp2"
(match_operand 2 "aarch64_pwr_imm3" "Up3"))
(match_operand 3 "const_int_operand" "n")
(const_int 0))
- (match_operand:GPI 4 "register_operand" "r"))
+ (match_operand:GPI 4 "register_operand" "rk"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
(plus:GPI (ANY_EXTRACT:GPI (mult:GPI (match_dup 1) (match_dup 2))
@@ -2259,7 +2259,7 @@ (define_insn "*adds_<optab><mode>_multp2"
(define_insn "*subs_<optab><mode>_multp2"
[(set (reg:CC_NZ CC_REGNUM)
(compare:CC_NZ
- (minus:GPI (match_operand:GPI 4 "register_operand" "r")
+ (minus:GPI (match_operand:GPI 4 "register_operand" "rk")
(ANY_EXTRACT:GPI
(mult:GPI (match_operand:GPI 1 "register_operand" "r")
(match_operand 2 "aarch64_pwr_imm3" "Up3"))
@@ -2923,7 +2923,7 @@ (define_insn "negvdi_carryinV"
(define_insn "*sub<mode>3_compare0"
[(set (reg:CC_NZ CC_REGNUM)
- (compare:CC_NZ (minus:GPI (match_operand:GPI 1 "register_operand" "r")
+ (compare:CC_NZ (minus:GPI (match_operand:GPI 1 "register_operand" "rk")
(match_operand:GPI 2 "register_operand" "r"))
(const_int 0)))
(set (match_operand:GPI 0 "register_operand" "=r")
@@ -2936,7 +2936,7 @@ (define_insn "*sub<mode>3_compare0"
;; zero_extend version of above
(define_insn "*subsi3_compare0_uxtw"
[(set (reg:CC_NZ CC_REGNUM)
- (compare:CC_NZ (minus:SI (match_operand:SI 1 "register_operand" "r")
+ (compare:CC_NZ (minus:SI (match_operand:SI 1 "register_operand" "rk")
(match_operand:SI 2 "register_operand" "r"))
(const_int 0)))
(set (match_operand:DI 0 "register_operand" "=r")
@@ -2949,7 +2949,7 @@ (define_insn "*subsi3_compare0_uxtw"
(define_insn "sub<mode>3_compare1_imm"
[(set (reg:CC CC_REGNUM)
(compare:CC
- (match_operand:GPI 1 "aarch64_reg_or_zero" "rZ,rZ")
+ (match_operand:GPI 1 "aarch64_reg_or_zero" "rkZ,rkZ")
(match_operand:GPI 2 "aarch64_plus_immediate" "I,J")))
(set (match_operand:GPI 0 "register_operand" "=r,r")
(plus:GPI
@@ -2965,7 +2965,7 @@ (define_insn "sub<mode>3_compare1_imm"
(define_insn "sub<mode>3_compare1"
[(set (reg:CC CC_REGNUM)
(compare:CC
- (match_operand:GPI 1 "aarch64_reg_or_zero" "rZ")
+ (match_operand:GPI 1 "aarch64_reg_or_zero" "rkZ")
(match_operand:GPI 2 "aarch64_reg_or_zero" "rZ")))
(set (match_operand:GPI 0 "register_operand" "=r")
(minus:GPI (match_dup 1) (match_dup 2)))]
@@ -2975,7 +2975,7 @@ (define_insn "sub<mode>3_compare1"
)
(define_peephole2
- [(set (match_operand:GPI 0 "register_operand")
+ [(set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_operand:GPI 1 "aarch64_reg_or_zero")
(match_operand:GPI 2 "aarch64_reg_or_zero")))
(set (reg:CC CC_REGNUM)
@@ -3000,7 +3000,7 @@ (define_peephole2
(compare:CC
(match_operand:GPI 1 "aarch64_reg_or_zero")
(match_operand:GPI 2 "aarch64_reg_or_zero")))
- (set (match_operand:GPI 0 "register_operand")
+ (set (match_operand:GPI 0 "aarch64_general_reg")
(minus:GPI (match_dup 1)
(match_dup 2)))]
""
@@ -3013,7 +3013,7 @@ (define_peephole2
)
(define_peephole2
- [(set (match_operand:GPI 0 "register_operand")
+ [(set (match_operand:GPI 0 "aarch64_general_reg")
(plus:GPI (match_operand:GPI 1 "register_operand")
(match_operand:GPI 2 "aarch64_plus_immediate")))
(set (reg:CC CC_REGNUM)
@@ -3038,7 +3038,7 @@ (define_peephole2
(compare:CC
(match_operand:GPI 1 "register_operand")
(match_operand:GPI 3 "const_int_operand")))
- (set (match_operand:GPI 0 "register_operand")
+ (set (match_operand:GPI 0 "aarch64_general_reg")
(plus:GPI (match_dup 1)
(match_operand:GPI 2 "aarch64_plus_immediate")))]
"INTVAL (operands[3]) == -INTVAL (operands[2])"
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index b8e6d232ff6237a58addda1ec0e953a1054dc616..8e1b784217b0367dc647a87024e14e36de781008 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -30,6 +30,10 @@ (define_predicate "aarch64_call_insn_operand"
(ior (match_code "symbol_ref")
(match_operand 0 "register_operand")))
+(define_predicate "aarch64_general_reg"
+ (and (match_operand 0 "register_operand")
+ (match_test "REGNO_REG_CLASS (REGNO (op)) == GENERAL_REGS")))
+
;; Return true if OP a (const_int 0) operand.
(define_predicate "const0_operand"
(and (match_code "const_int")
diff --git a/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c b/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c
new file mode 100644
index 0000000000000000000000000000000000000000..f537771a271e1d33df29561a4b687d621e090bab
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/rtl/aarch64/subs_adds_sp.c
@@ -0,0 +1,153 @@
+/* { dg-do compile { target aarch64-*-* } } */
+/* { dg-options "-O2" } */
+/*
+ Tests are:
+ Patterns allow subs/adds with a stack pointer source.
+ define_peephole2's don't generate patterns for subs/adds with a stack
+ pointer destination.
+ */
+
+/* These functions used to ICE due to using the stack pointer as a source
+ register. */
+
+int __RTL (startwith ("final"))
+adds ()
+{
+(function "adds"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 101 (parallel [
+ (set (reg:CC cc)
+ (compare:CC (reg/f:DI sp)
+ (const_int -3)))
+ (set (reg/f:DI x19)
+ (plus:DI (reg/f:DI sp)
+ (const_int 3)))
+ ]))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 10 (use (reg/i:SI x19)))
+ (cinsn 11 (use (reg/i:SI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("final"))
+subs ()
+{
+(function "subs"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 101 (parallel [
+ (set (reg:CC cc)
+ (compare:CC (reg/f:DI sp)
+ (const_int 3)))
+ (set (reg/f:DI x19)
+ (plus:DI (reg/f:DI sp)
+ (const_int -3)))
+ ]))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 10 (use (reg/i:SI x19)))
+ (cinsn 11 (use (reg/i:SI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+/* These functions used to trigger peepholes generating invalid SUBS patterns
+ that used the stack pointer for the destination register. */
+
+int __RTL (startwith ("peephole2")) sub3_compare1_peephole_1 ()
+{
+(function "sub3_compare1_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_peephole_2 ()
+{
+(function "sub3_compare1_peephole_2"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_imm_peephole_1 ()
+{
+(function "sub3_compare1_imm_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+int __RTL (startwith ("peephole2")) sub3_compare1_imm_peephole_2 ()
+{
+(function "sub3_compare1_imm_peephole_1"
+ (insn-chain
+ (block 2
+ (edge-from entry (flags "FALLTHRU"))
+ (cnote 3 [bb 2] NOTE_INSN_BASIC_BLOCK)
+ (cinsn 89 (set (reg:DI sp)
+ (minus:DI (reg:DI x2) (reg:DI x5))))
+ (cinsn 90 (set (reg:CC cc)
+ (compare:CC (reg:DI x2) (reg:DI x5))))
+ ;; Extra insn to avoid the above being deleted by DCE.
+ (cinsn 12 (use (reg/i:DI cc)))
+ (cinsn 11 (use (reg/i:DI sp)))
+ (edge-to exit (flags "FALLTHRU"))
+ ) ;; block 2
+ ) ;; insn-chain
+) ;; function "main"
+}
+
+/* Verify that the adds and subs functions generated their respective
+ instructions, and that none of the other functions generated either since
+ they are setting the stack pointer. */
+/* { dg-final { scan-assembler-times {adds\tx[0-9]+, sp} 1 } } */
+/* { dg-final { scan-assembler-not {adds\tsp} } } */
+/* { dg-final { scan-assembler-times {subs\tx[0-9]+, sp} 1 } } */
+/* { dg-final { scan-assembler-not {subs\tsp} } } */
+
diff --git a/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90 b/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90
new file mode 100644
index 0000000000000000000000000000000000000000..014b655d5edae26689ff57eda808e2a0f6146d5b
--- /dev/null
+++ b/gcc/testsuite/gfortran.fortran-torture/compile/pr89324.f90
@@ -0,0 +1,15 @@
+module a
+contains
+ pure function myotherlen()
+ myotherlen = 99
+ end
+ subroutine b
+ characterx
+ block
+ character(myotherlen()) c
+ c = "abc"
+ x = c
+ end block
+
+ end
+end
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions
2019-02-22 16:11 ` Matthew Malcomson
@ 2019-02-22 16:25 ` James Greenhalgh
0 siblings, 0 replies; 5+ messages in thread
From: James Greenhalgh @ 2019-02-22 16:25 UTC (permalink / raw)
To: Matthew Malcomson; +Cc: gcc-patches, Richard Earnshaw, nd, Marcus Shawcroft
On Fri, Feb 22, 2019 at 09:39:59AM -0600, Matthew Malcomson wrote:
> Hi James,
>
> On 22/02/19 00:09, James Greenhalgh wrote:
> > On Mon, Feb 18, 2019 at 08:40:12AM -0600, Matthew Malcomson wrote:
> >>
> >> Additionally, this patch contains two tidy-ups (happy to remove them or put in
> >> a separate patch if people want):
> >
> > Generally, yes I prefer that.
> >
>
> I've removed the tidy ups and retested -- modified patch attached.
>
> >>
> >> OK for trunk?
> >
> > This patch is fine for GCC 10, so not on trunk yet please unless there is
> > a corectness reason for the change.
> >
>
> The patch is a correctness change to fix the P1 regression that occurs
> when the stack pointer was passed to the define_insn from the
> define_peephole2.
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89324
>
> Ok for trunk?
Yes, please.
Thanks,
James
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-22 16:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-18 14:40 [Patch] [aarch64] PR target/89324 Handle stack pointer for SUBS/ADDS instructions Matthew Malcomson
2019-02-22 0:48 ` James Greenhalgh
2019-02-22 6:47 ` Mike Stump
2019-02-22 16:11 ` Matthew Malcomson
2019-02-22 16:25 ` James Greenhalgh
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).