Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes - remove q constraint
On 2/20/19 9:12 PM, Jakub Jelinek wrote: On Mon, Feb 18, 2019 at 12:47:04PM +, Kyrill Tkachov wrote: Ok. Thanks for working on this. Sorry for the endless story here, but I've realized that the *arm_ldrd and *arm_strd instructions are the only remaining uses of the undocumented internal constraint q and that it isn't really needed even for these instructions, we can just use rk instead of q. Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk? Ok. Thanks, Kyrill 2019-02-20 Jakub Jelinek PR bootstrap/88714 * constraints.md (q): Remove. * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use rk constraint instead of q. --- gcc/config/arm/constraints.md.jj2019-01-01 12:37:27.032812929 +0100 +++ gcc/config/arm/constraints.md 2019-02-18 20:18:51.816941795 +0100 @@ -90,9 +90,6 @@ (define_constraint "PJ" (define_register_constraint "k" "STACK_REG" "@internal The stack register.") -(define_register_constraint "q" "(TARGET_ARM && TARGET_LDRD) ? CORE_REGS : GENERAL_REGS" - "@internal In ARM state with LDRD support, core registers, otherwise general registers.") - (define_register_constraint "b" "TARGET_THUMB ? BASE_REGS : NO_REGS" "@internal Thumb only. The union of the low registers and the stack register.") --- gcc/config/arm/ldrdstrd.md.jj 2019-02-18 20:19:34.976233961 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-18 20:19:54.555912842 +0100 @@ -159,7 +159,7 @@ (define_peephole2 ; swap the destination (define_insn "*arm_ldrd" [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") (match_operand:SI 2 "memory_operand" "m")) - (set (match_operand:SI 1 "s_register_operand" "=q") + (set (match_operand:SI 1 "s_register_operand" "=rk") (match_operand:SI 3 "memory_operand" "m"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, true)" @@ -180,7 +180,7 @@ (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") (match_operand:SI 0 "s_register_operand" "r")) (set (match_operand:SI 3 "memory_operand" "=m") - (match_operand:SI 1 "s_register_operand" "q"))])] + (match_operand:SI 1 "s_register_operand" "rk"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, false)" { Jakub
[Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes - remove q constraint
On Mon, Feb 18, 2019 at 12:47:04PM +, Kyrill Tkachov wrote: > Ok. > > Thanks for working on this. Sorry for the endless story here, but I've realized that the *arm_ldrd and *arm_strd instructions are the only remaining uses of the undocumented internal constraint q and that it isn't really needed even for these instructions, we can just use rk instead of q. Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk? 2019-02-20 Jakub Jelinek PR bootstrap/88714 * constraints.md (q): Remove. * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use rk constraint instead of q. --- gcc/config/arm/constraints.md.jj2019-01-01 12:37:27.032812929 +0100 +++ gcc/config/arm/constraints.md 2019-02-18 20:18:51.816941795 +0100 @@ -90,9 +90,6 @@ (define_constraint "PJ" (define_register_constraint "k" "STACK_REG" "@internal The stack register.") -(define_register_constraint "q" "(TARGET_ARM && TARGET_LDRD) ? CORE_REGS : GENERAL_REGS" - "@internal In ARM state with LDRD support, core registers, otherwise general registers.") - (define_register_constraint "b" "TARGET_THUMB ? BASE_REGS : NO_REGS" "@internal Thumb only. The union of the low registers and the stack register.") --- gcc/config/arm/ldrdstrd.md.jj 2019-02-18 20:19:34.976233961 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-18 20:19:54.555912842 +0100 @@ -159,7 +159,7 @@ (define_peephole2 ; swap the destination (define_insn "*arm_ldrd" [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") (match_operand:SI 2 "memory_operand" "m")) - (set (match_operand:SI 1 "s_register_operand" "=q") + (set (match_operand:SI 1 "s_register_operand" "=rk") (match_operand:SI 3 "memory_operand" "m"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, true)" @@ -180,7 +180,7 @@ (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") (match_operand:SI 0 "s_register_operand" "r")) (set (match_operand:SI 3 "memory_operand" "=m") - (match_operand:SI 1 "s_register_operand" "q"))])] + (match_operand:SI 1 "s_register_operand" "rk"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, false)" { Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On 2/17/19 7:29 AM, Jakub Jelinek wrote: On Mon, Feb 11, 2019 at 12:08:32PM +0100, Jakub Jelinek wrote: So like the patch below (though, I have only limited possibilities to test this, can throw it in armv7hl-linux-gnueabi distro build). Actually, that patch was bad, I misread the CORE_REGS vs. GENERAL_REGS hardregset difference, it is actually sp that is not GENERAL_REGS but is CORE_REGS, not ip. So here is an updated patch, same except that in ldrdstrd.md the q constraints are kept in the right spot. To repeat, I don't think the q constraints on movdi are now needed, because ldrdstrd doesn't use those DImode patterns and RA will not allocate a DImode hard reg starting at ip because sp is a fixed register. Bootstrapped/regtested on armv7hl-linux-gnueabi (distro build), ok for trunk? Ok. Thanks for working on this. Kyrill 2019-02-17 Jakub Jelinek PR bootstrap/88714 * config/arm/arm.md (*arm_movdi, *movdf_soft_insn): Use "r" instead of "q" constraint. * config/arm/vfp.md (*movdi_vfp): Likewise. * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use "r" instead of "q" constraint for operands[0]. --- gcc/config/arm/arm.md.jj2019-01-31 00:26:04.417738975 +0100 +++ gcc/config/arm/arm.md 2019-02-11 12:02:32.778707056 +0100 @@ -5817,8 +5817,8 @@ (define_expand "movdi" ) (define_insn "*arm_movdi" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, q, m") - (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,q"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m") + (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,r"))] "TARGET_32BIT && !(TARGET_HARD_FLOAT) && !TARGET_IWMMXT @@ -7102,8 +7102,8 @@ (define_expand "reload_outdf" ) (define_insn "*movdf_soft_insn" - [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,q,m") - (match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,q"))] + [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,r,m") + (match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,r"))] "TARGET_32BIT && TARGET_SOFT_FLOAT && ( register_operand (operands[0], DFmode) || register_operand (operands[1], DFmode))" --- gcc/config/arm/vfp.md.jj2019-01-31 00:26:04.312740661 +0100 +++ gcc/config/arm/vfp.md 2019-02-11 12:03:13.232045976 +0100 @@ -307,8 +307,8 @@ (define_insn "*thumb2_movsi_vfp" ;; DImode moves (define_insn "*movdi_vfp" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r,r,r,r,q,q,m,w,!r,w,w, Uv") - (match_operand:DI 1 "di_operand" "r,rDa,Db,Dc,mi,mi,q,r,w,w,UvTu,w"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r,r,r,r,r,r,m,w,!r,w,w, Uv") + (match_operand:DI 1 "di_operand" "r,rDa,Db,Dc,mi,mi,r,r,w,w,UvTu,w"))] "TARGET_32BIT && TARGET_HARD_FLOAT && ( register_operand (operands[0], DImode) || register_operand (operands[1], DImode)) --- gcc/config/arm/ldrdstrd.md.jj 2019-02-11 11:39:39.977125795 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-11 12:03:57.978314745 +0100 @@ -157,7 +157,7 @@ ;; We use gen_operands_ldrd_strd() with a modify argument as false so that the ;; operands are not changed. (define_insn "*arm_ldrd" - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") (match_operand:SI 2 "memory_operand" "m")) (set (match_operand:SI 1 "s_register_operand" "=q") (match_operand:SI 3 "memory_operand" "m"))])] @@ -178,7 +178,7 @@ (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") - (match_operand:SI 0 "s_register_operand" "q")) + (match_operand:SI 0 "s_register_operand" "r")) (set (match_operand:SI 3 "memory_operand" "=m") (match_operand:SI 1 "s_register_operand" "q"))])] "TARGET_LDRD && TARGET_ARM && reload_completed Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Mon, Feb 11, 2019 at 12:08:32PM +0100, Jakub Jelinek wrote: > So like the patch below (though, I have only limited possibilities to test > this, can throw it in armv7hl-linux-gnueabi distro build). Actually, that patch was bad, I misread the CORE_REGS vs. GENERAL_REGS hardregset difference, it is actually sp that is not GENERAL_REGS but is CORE_REGS, not ip. So here is an updated patch, same except that in ldrdstrd.md the q constraints are kept in the right spot. To repeat, I don't think the q constraints on movdi are now needed, because ldrdstrd doesn't use those DImode patterns and RA will not allocate a DImode hard reg starting at ip because sp is a fixed register. Bootstrapped/regtested on armv7hl-linux-gnueabi (distro build), ok for trunk? 2019-02-17 Jakub Jelinek PR bootstrap/88714 * config/arm/arm.md (*arm_movdi, *movdf_soft_insn): Use "r" instead of "q" constraint. * config/arm/vfp.md (*movdi_vfp): Likewise. * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use "r" instead of "q" constraint for operands[0]. --- gcc/config/arm/arm.md.jj2019-01-31 00:26:04.417738975 +0100 +++ gcc/config/arm/arm.md 2019-02-11 12:02:32.778707056 +0100 @@ -5817,8 +5817,8 @@ (define_expand "movdi" ) (define_insn "*arm_movdi" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, q, m") - (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,q"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m") + (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,r"))] "TARGET_32BIT && !(TARGET_HARD_FLOAT) && !TARGET_IWMMXT @@ -7102,8 +7102,8 @@ (define_expand "reload_outdf" ) (define_insn "*movdf_soft_insn" - [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,q,m") - (match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,q"))] + [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,r,m") + (match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,r"))] "TARGET_32BIT && TARGET_SOFT_FLOAT && ( register_operand (operands[0], DFmode) || register_operand (operands[1], DFmode))" --- gcc/config/arm/vfp.md.jj2019-01-31 00:26:04.312740661 +0100 +++ gcc/config/arm/vfp.md 2019-02-11 12:03:13.232045976 +0100 @@ -307,8 +307,8 @@ (define_insn "*thumb2_movsi_vfp" ;; DImode moves (define_insn "*movdi_vfp" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r,r,r,r,q,q,m,w,!r,w,w, Uv") - (match_operand:DI 1 "di_operand" "r,rDa,Db,Dc,mi,mi,q,r,w,w,UvTu,w"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r,r,r,r,r,r,m,w,!r,w,w, Uv") + (match_operand:DI 1 "di_operand" "r,rDa,Db,Dc,mi,mi,r,r,w,w,UvTu,w"))] "TARGET_32BIT && TARGET_HARD_FLOAT && ( register_operand (operands[0], DImode) || register_operand (operands[1], DImode)) --- gcc/config/arm/ldrdstrd.md.jj 2019-02-11 11:39:39.977125795 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-11 12:03:57.978314745 +0100 @@ -157,7 +157,7 @@ ;; We use gen_operands_ldrd_strd() with a modify argument as false so that the ;; operands are not changed. (define_insn "*arm_ldrd" - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") (match_operand:SI 2 "memory_operand" "m")) (set (match_operand:SI 1 "s_register_operand" "=q") (match_operand:SI 3 "memory_operand" "m"))])] @@ -178,7 +178,7 @@ (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") - (match_operand:SI 0 "s_register_operand" "q")) + (match_operand:SI 0 "s_register_operand" "r")) (set (match_operand:SI 3 "memory_operand" "=m") (match_operand:SI 1 "s_register_operand" "q"))])] "TARGET_LDRD && TARGET_ARM && reload_completed Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On 2/11/19 2:35 PM, Matthew Malcomson wrote: On 10/02/19 09:42, Christophe Lyon wrote: > > Both this simple patch or the previous fix all the ICEs I reported, thanks. > > Of course, the scan-assembler failures remain to be fixed. > In the testcase I failed to account for targets that don't support arm mode or targets that do not support the ldrd/strd instructions. This patch accounts for both of these by adding some dg-require-effective-target lines to the testcase. This patch also adds a new effective-target procedure to check a target supports arm ldrd/strd. This check uses the 'r' constraint to ensure SP is not used so that it will work for thumb mode code generation as well as arm mode. Tested by running this testcase with cross compilers using "-march=armv5t", "-mcpu=cortex-m3", "-mcpu-arm7tdmi", "-mcpu=cortex-a9 -march=armv5t" for both arm-none-eabi and arm-none-linux-gnueabihf. Also ran this testcase with `make check` natively. Ok for trunk? Ok. Thanks, Kyrill gcc/testsuite/ChangeLog: 2019-02-11 Matthew Malcomson * gcc.dg/rtl/arm/ldrd-peepholes.c: Restrict testcase. * lib/target-supports.exp: Add procedure to check for ldrd. diff --git a/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c b/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c index 4c3949c0963b8482545df670c31db2d9ec0f26b3..cbb64a770f5d796250601cafe481d7c2ea13f2eb 100644 --- a/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c +++ b/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c @@ -1,4 +1,6 @@ /* { dg-do compile { target arm*-*-* } } */ +/* { dg-require-effective-target arm_arm_ok } */ +/* { dg-require-effective-target arm_ldrd_strd_ok } */ /* { dg-skip-if "Ensure only targetting arm with TARGET_LDRD" { *-*-* } { "-mthumb" } { "" } } */ /* { dg-options "-O3 -marm -fdump-rtl-peephole2" } */ diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index a0b4b99067f9ae225bde3b6bc719e89e1ea8e0e1..16dd018e8020fdf8e104690fed6a4e8919aa4aa1 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -4918,6 +4918,27 @@ proc check_effective_target_arm_prefer_ldrd_strd { } { } "-O2 -mthumb" ] } +# Return true if LDRD/STRD instructions are available on this target. +proc check_effective_target_arm_ldrd_strd_ok { } { + if { ![check_effective_target_arm32] } { + return 0; + } + + return [check_no_compiler_messages arm_ldrd_strd_ok object { + int main(void) + { + __UINT64_TYPE__ a = 1, b = 10; + __UINT64_TYPE__ *c = &b; + // `a` will be in a valid register since it's a DImode quantity. + asm ("ldrd %0, %1" + : "=r" (a) + : "m" (c)); + return a == 10; + } + }] +} + # Return 1 if this is a PowerPC target supporting -meabi. proc check_effective_target_powerpc_eabi_ok { } {
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On 10/02/19 09:42, Christophe Lyon wrote: > > Both this simple patch or the previous fix all the ICEs I reported, thanks. > > Of course, the scan-assembler failures remain to be fixed. > In the testcase I failed to account for targets that don't support arm mode or targets that do not support the ldrd/strd instructions. This patch accounts for both of these by adding some dg-require-effective-target lines to the testcase. This patch also adds a new effective-target procedure to check a target supports arm ldrd/strd. This check uses the 'r' constraint to ensure SP is not used so that it will work for thumb mode code generation as well as arm mode. Tested by running this testcase with cross compilers using "-march=armv5t", "-mcpu=cortex-m3", "-mcpu-arm7tdmi", "-mcpu=cortex-a9 -march=armv5t" for both arm-none-eabi and arm-none-linux-gnueabihf. Also ran this testcase with `make check` natively. Ok for trunk? gcc/testsuite/ChangeLog: 2019-02-11 Matthew Malcomson * gcc.dg/rtl/arm/ldrd-peepholes.c: Restrict testcase. * lib/target-supports.exp: Add procedure to check for ldrd. diff --git a/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c b/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c index 4c3949c0963b8482545df670c31db2d9ec0f26b3..cbb64a770f5d796250601cafe481d7c2ea13f2eb 100644 --- a/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c +++ b/gcc/testsuite/gcc.dg/rtl/arm/ldrd-peepholes.c @@ -1,4 +1,6 @@ /* { dg-do compile { target arm*-*-* } } */ +/* { dg-require-effective-target arm_arm_ok } */ +/* { dg-require-effective-target arm_ldrd_strd_ok } */ /* { dg-skip-if "Ensure only targetting arm with TARGET_LDRD" { *-*-* } { "-mthumb" } { "" } } */ /* { dg-options "-O3 -marm -fdump-rtl-peephole2" } */ diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index a0b4b99067f9ae225bde3b6bc719e89e1ea8e0e1..16dd018e8020fdf8e104690fed6a4e8919aa4aa1 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -4918,6 +4918,27 @@ proc check_effective_target_arm_prefer_ldrd_strd { } { } "-O2 -mthumb" ] } +# Return true if LDRD/STRD instructions are available on this target. +proc check_effective_target_arm_ldrd_strd_ok { } { +if { ![check_effective_target_arm32] } { + return 0; +} + +return [check_no_compiler_messages arm_ldrd_strd_ok object { + int main(void) + { +__UINT64_TYPE__ a = 1, b = 10; +__UINT64_TYPE__ *c = &b; +// `a` will be in a valid register since it's a DImode quantity. +asm ("ldrd %0, %1" + : "=r" (a) + : "m" (c)); +return a == 10; + } +}] +} + # Return 1 if this is a PowerPC target supporting -meabi. proc check_effective_target_powerpc_eabi_ok { } {
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Mon, Feb 11, 2019 at 10:32:23AM +, Kyrill Tkachov wrote: > I think this is ok. Ok, committed the simpler version. > The "q" constraint was introduced after the iwmmxt.md patterns were written > and it seems > that they were just never updated to use it. > It's hard for anyone to get a hold of the relevant hardware to test iwmmxt > these days, > so I suspect that path hasn't been thoroughly tested. > In my opinion the ldrd/strd-related logic there should follow the same > approach as the rest of > the arm backend, that is, using 'q'. > > A patch to update the iwmmxt.md constraints in that way is pre-approved. Thinking about it some more, given that the "q" constraint has been introduced exactly for the ldrdstrd.md created movdi patterns (https://gcc.gnu.org/ml/gcc-patches/2013-02/msg00604.html), we don't generate those anymore, and r13 aka sp is a FIXED_REGISTERS, I wonder if instead of that we shouldn't change *arm_movdi and *movdi_vfp patterns (what about *movdf_soft_insn ?) to use r constraint again - the RA shouldn't be putting DImode regs at ip, because only half of that register is non-fixed and previously the only way to get there was ldrdstrd peephole2s, but those use *arm_ldrd/*arm_strd patterns now. So like the patch below (though, I have only limited possibilities to test this, can throw it in armv7hl-linux-gnueabi distro build). 2019-02-11 Jakub Jelinek PR bootstrap/88714 * config/arm/arm.md (*arm_movdi, *movdf_soft_insn): Use "r" instead of "q" constraint. * config/arm/vfp.md (*movdi_vfp): Likewise. * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use "r" instead of "q" constraint for operands[1]. --- gcc/config/arm/arm.md.jj2019-01-31 00:26:04.417738975 +0100 +++ gcc/config/arm/arm.md 2019-02-11 12:02:32.778707056 +0100 @@ -5817,8 +5817,8 @@ (define_expand "movdi" ) (define_insn "*arm_movdi" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, q, m") - (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,q"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m") + (match_operand:DI 1 "di_operand" "rDa,Db,Dc,mi,r"))] "TARGET_32BIT && !(TARGET_HARD_FLOAT) && !TARGET_IWMMXT @@ -7102,8 +7102,8 @@ (define_expand "reload_outdf" ) (define_insn "*movdf_soft_insn" - [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,q,m") - (match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,q"))] + [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,r,m") + (match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,r"))] "TARGET_32BIT && TARGET_SOFT_FLOAT && ( register_operand (operands[0], DFmode) || register_operand (operands[1], DFmode))" --- gcc/config/arm/vfp.md.jj2019-01-31 00:26:04.312740661 +0100 +++ gcc/config/arm/vfp.md 2019-02-11 12:03:13.232045976 +0100 @@ -307,8 +307,8 @@ (define_insn "*thumb2_movsi_vfp" ;; DImode moves (define_insn "*movdi_vfp" - [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r,r,r,r,q,q,m,w,!r,w,w, Uv") - (match_operand:DI 1 "di_operand" "r,rDa,Db,Dc,mi,mi,q,r,w,w,UvTu,w"))] + [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r,r,r,r,r,r,m,w,!r,w,w, Uv") + (match_operand:DI 1 "di_operand" "r,rDa,Db,Dc,mi,mi,r,r,w,w,UvTu,w"))] "TARGET_32BIT && TARGET_HARD_FLOAT && ( register_operand (operands[0], DImode) || register_operand (operands[1], DImode)) --- gcc/config/arm/ldrdstrd.md.jj 2019-02-11 11:39:39.977125795 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-11 12:03:57.978314745 +0100 @@ -159,7 +159,7 @@ (define_peephole2 ; swap the destination (define_insn "*arm_ldrd" [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") (match_operand:SI 2 "memory_operand" "m")) - (set (match_operand:SI 1 "s_register_operand" "=q") + (set (match_operand:SI 1 "s_register_operand" "=r") (match_operand:SI 3 "memory_operand" "m"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, true)" @@ -180,7 +180,7 @@ (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") (match_operand:SI 0 "s_register_operand" "q")) (set (match_operand:SI 3 "memory_operand" "=m") - (match_operand:SI 1 "s_register_operand" "q"))])] + (match_operand:SI 1 "s_register_operand" "r"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, false)" { Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
Hi Jakub, On 08/02/19 11:40, Jakub Jelinek wrote: On Fri, Feb 08, 2019 at 11:29:10AM +, Matthew Malcomson wrote: > I'm pretty sure there's no difference between the iwmmxt target and > others so believe your simpler fix of just using 'q' is a good idea. > (there's no difference in gas and no documentation I have found mentions > a difference). The simpler patch would be then (but of course in that case the question is why iwmmxt.md doesn't use those q constraints for the output_move_double alternatives). I think this is ok. The "q" constraint was introduced after the iwmmxt.md patterns were written and it seems that they were just never updated to use it. It's hard for anyone to get a hold of the relevant hardware to test iwmmxt these days, so I suspect that path hasn't been thoroughly tested. In my opinion the ldrd/strd-related logic there should follow the same approach as the rest of the arm backend, that is, using 'q'. A patch to update the iwmmxt.md constraints in that way is pre-approved. Thanks, Kyrill 2019-02-08 Jakub Jelinek PR bootstrap/88714 * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use q constraint instead of r. --- gcc/config/arm/ldrdstrd.md.jj 2019-02-08 11:25:42.368916124 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-08 12:38:33.647585108 +0100 @@ -157,9 +157,9 @@ (define_peephole2 ; swap the destination ;; We use gen_operands_ldrd_strd() with a modify argument as false so that the ;; operands are not changed. (define_insn "*arm_ldrd" - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") (match_operand:SI 2 "memory_operand" "m")) - (set (match_operand:SI 1 "s_register_operand" "=r") + (set (match_operand:SI 1 "s_register_operand" "=q") (match_operand:SI 3 "memory_operand" "m"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, true)" @@ -178,9 +178,9 @@ (define_insn "*arm_ldrd" (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") - (match_operand:SI 0 "s_register_operand" "r")) + (match_operand:SI 0 "s_register_operand" "q")) (set (match_operand:SI 3 "memory_operand" "=m") - (match_operand:SI 1 "s_register_operand" "r"))])] + (match_operand:SI 1 "s_register_operand" "q"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, false)" { Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Sun, Feb 10, 2019 at 10:42:55AM +0100, Christophe Lyon wrote: > > 2019-02-08 Jakub Jelinek > > > > PR bootstrap/88714 > > * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use q constraint > > instead of r. > > > > Both this simple patch or the previous fix all the ICEs I reported, thanks. > > Of course, the scan-assembler failures remain to be fixed. Thanks. Is the patch ok for trunk then (which one)? There is yet another variant I guess, using =q constraint just on the operand 0, because valid_operands_ldrd_strd requires that the first reg is even and second one higher and as the only difference between q and r (CORE_REGS vs. GENERAL_REGS) is the ip register which has regno 12, the second operand must not be ip anyway. > > --- gcc/config/arm/ldrdstrd.md.jj 2019-02-08 11:25:42.368916124 +0100 > > +++ gcc/config/arm/ldrdstrd.md 2019-02-08 12:38:33.647585108 +0100 > > @@ -157,9 +157,9 @@ (define_peephole2 ; swap the destination > > ;; We use gen_operands_ldrd_strd() with a modify argument as false so that > > the > > ;; operands are not changed. > > (define_insn "*arm_ldrd" > > - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") > > + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") > >(match_operand:SI 2 "memory_operand" "m")) > > - (set (match_operand:SI 1 "s_register_operand" "=r") > > + (set (match_operand:SI 1 "s_register_operand" "=q") > >(match_operand:SI 3 "memory_operand" "m"))])] > >"TARGET_LDRD && TARGET_ARM && reload_completed > >&& valid_operands_ldrd_strd (operands, true)" > > @@ -178,9 +178,9 @@ (define_insn "*arm_ldrd" > > > > (define_insn "*arm_strd" > >[(parallel [(set (match_operand:SI 2 "memory_operand" "=m") > > - (match_operand:SI 0 "s_register_operand" "r")) > > + (match_operand:SI 0 "s_register_operand" "q")) > > (set (match_operand:SI 3 "memory_operand" "=m") > > - (match_operand:SI 1 "s_register_operand" "r"))])] > > + (match_operand:SI 1 "s_register_operand" "q"))])] > >"TARGET_LDRD && TARGET_ARM && reload_completed > >&& valid_operands_ldrd_strd (operands, false)" > >{ Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Fri, 8 Feb 2019 at 12:40, Jakub Jelinek wrote: > > On Fri, Feb 08, 2019 at 11:29:10AM +, Matthew Malcomson wrote: > > I'm pretty sure there's no difference between the iwmmxt target and > > others so believe your simpler fix of just using 'q' is a good idea. > > (there's no difference in gas and no documentation I have found mentions > > a difference). > > The simpler patch would be then (but of course in that case the question is > why iwmmxt.md doesn't use those q constraints for the output_move_double > alternatives). > > 2019-02-08 Jakub Jelinek > > PR bootstrap/88714 > * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use q constraint > instead of r. > Both this simple patch or the previous fix all the ICEs I reported, thanks. Of course, the scan-assembler failures remain to be fixed. > --- gcc/config/arm/ldrdstrd.md.jj 2019-02-08 11:25:42.368916124 +0100 > +++ gcc/config/arm/ldrdstrd.md 2019-02-08 12:38:33.647585108 +0100 > @@ -157,9 +157,9 @@ (define_peephole2 ; swap the destination > ;; We use gen_operands_ldrd_strd() with a modify argument as false so that > the > ;; operands are not changed. > (define_insn "*arm_ldrd" > - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") > + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") >(match_operand:SI 2 "memory_operand" "m")) > - (set (match_operand:SI 1 "s_register_operand" "=r") > + (set (match_operand:SI 1 "s_register_operand" "=q") >(match_operand:SI 3 "memory_operand" "m"))])] >"TARGET_LDRD && TARGET_ARM && reload_completed >&& valid_operands_ldrd_strd (operands, true)" > @@ -178,9 +178,9 @@ (define_insn "*arm_ldrd" > > (define_insn "*arm_strd" >[(parallel [(set (match_operand:SI 2 "memory_operand" "=m") > - (match_operand:SI 0 "s_register_operand" "r")) > + (match_operand:SI 0 "s_register_operand" "q")) > (set (match_operand:SI 3 "memory_operand" "=m") > - (match_operand:SI 1 "s_register_operand" "r"))])] > + (match_operand:SI 1 "s_register_operand" "q"))])] >"TARGET_LDRD && TARGET_ARM && reload_completed >&& valid_operands_ldrd_strd (operands, false)" >{ > > > Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Fri, Feb 08, 2019 at 11:29:10AM +, Matthew Malcomson wrote: > I'm pretty sure there's no difference between the iwmmxt target and > others so believe your simpler fix of just using 'q' is a good idea. > (there's no difference in gas and no documentation I have found mentions > a difference). The simpler patch would be then (but of course in that case the question is why iwmmxt.md doesn't use those q constraints for the output_move_double alternatives). 2019-02-08 Jakub Jelinek PR bootstrap/88714 * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Use q constraint instead of r. --- gcc/config/arm/ldrdstrd.md.jj 2019-02-08 11:25:42.368916124 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-08 12:38:33.647585108 +0100 @@ -157,9 +157,9 @@ (define_peephole2 ; swap the destination ;; We use gen_operands_ldrd_strd() with a modify argument as false so that the ;; operands are not changed. (define_insn "*arm_ldrd" - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q") (match_operand:SI 2 "memory_operand" "m")) - (set (match_operand:SI 1 "s_register_operand" "=r") + (set (match_operand:SI 1 "s_register_operand" "=q") (match_operand:SI 3 "memory_operand" "m"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, true)" @@ -178,9 +178,9 @@ (define_insn "*arm_ldrd" (define_insn "*arm_strd" [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") - (match_operand:SI 0 "s_register_operand" "r")) + (match_operand:SI 0 "s_register_operand" "q")) (set (match_operand:SI 3 "memory_operand" "=m") - (match_operand:SI 1 "s_register_operand" "r"))])] + (match_operand:SI 1 "s_register_operand" "q"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, false)" { Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On 08/02/19 10:23, Jakub Jelinek wrote: > On Fri, Feb 08, 2019 at 11:06:02AM +0100, Christophe Lyon wrote: >> On Fri, 8 Feb 2019 at 10:51, Jakub Jelinek wrote: >>> >>> On Fri, Feb 08, 2019 at 10:18:03AM +0100, Christophe Lyon wrote: I'm afaid this patch causes several regressions. Maybe they have already been fixed post-commit (I have several validations for later commits still running)? >>> >>> The following patch fixes the single ICE I've tried to reproduce. >>> While iwmmxt.md movdi pattern uses =m, r and =r, m constraints, both >>> arm.md and vfp.md movdi patterns use =m, q and =q, m constraints and thus >>> allow also ip register. The following patch is an attempt to do the same >>> thing, just in the same patterns through arch_enabled attribute. >>> >>> Completely untested. >>> >>> 2019-02-08 Jakub Jelinek >>> >>> PR bootstrap/88714 >>> * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Add alternative >>> with >>> q constraint instead of r, enable it only if not >>> TARGET_REALLY_IWMMXT. >> >> I've started validations with this patch, I expect the results later today. > > Thanks. Note, I don't understand why iwmmxt.md doesn't use q constraint, if > it is only some omission, or some hw requirement. So, the patch just > follows what iwmmxt.md does. I guess because iwmmxt.md movsi also uses r > constraint, ip really shouldn't appear on that target. But if just changing > all r constraints to q without any arch_enabled works, that would be even > simpler. > > Jakub > Apologies for the break to everyone, and thanks Jakub for the fix. I believe my mistake was that I didn't match the behaviour of the function `operands_ok_ldrd_strd` (that the peepholes use) and the constraints: so the peepholes allow using ip while the insn patterns don't. I'm pretty sure there's no difference between the iwmmxt target and others so believe your simpler fix of just using 'q' is a good idea. (there's no difference in gas and no documentation I have found mentions a difference). I had decided to use 'r' instead of 'q' based on the phrase "ARM strongly recommends that you do not use R12 for Rt." under the "doubleword register restrictions" heading in the link below http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0802a/CIHGJHED.html But don't think the statement is something that *must* be followed, since it's a recommendation in the same list as hard requirements. There's no mention of this in any ARM ARM that I can find. Matthew
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Fri, Feb 08, 2019 at 11:06:02AM +0100, Christophe Lyon wrote: > On Fri, 8 Feb 2019 at 10:51, Jakub Jelinek wrote: > > > > On Fri, Feb 08, 2019 at 10:18:03AM +0100, Christophe Lyon wrote: > > > I'm afaid this patch causes several regressions. Maybe they have > > > already been fixed post-commit (I have several validations for later > > > commits still running)? > > > > The following patch fixes the single ICE I've tried to reproduce. > > While iwmmxt.md movdi pattern uses =m, r and =r, m constraints, both > > arm.md and vfp.md movdi patterns use =m, q and =q, m constraints and thus > > allow also ip register. The following patch is an attempt to do the same > > thing, just in the same patterns through arch_enabled attribute. > > > > Completely untested. > > > > 2019-02-08 Jakub Jelinek > > > > PR bootstrap/88714 > > * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Add alternative > > with > > q constraint instead of r, enable it only if not > > TARGET_REALLY_IWMMXT. > > I've started validations with this patch, I expect the results later today. Thanks. Note, I don't understand why iwmmxt.md doesn't use q constraint, if it is only some omission, or some hw requirement. So, the patch just follows what iwmmxt.md does. I guess because iwmmxt.md movsi also uses r constraint, ip really shouldn't appear on that target. But if just changing all r constraints to q without any arch_enabled works, that would be even simpler. Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Fri, 8 Feb 2019 at 10:51, Jakub Jelinek wrote: > > On Fri, Feb 08, 2019 at 10:18:03AM +0100, Christophe Lyon wrote: > > I'm afaid this patch causes several regressions. Maybe they have > > already been fixed post-commit (I have several validations for later > > commits still running)? > > The following patch fixes the single ICE I've tried to reproduce. > While iwmmxt.md movdi pattern uses =m, r and =r, m constraints, both > arm.md and vfp.md movdi patterns use =m, q and =q, m constraints and thus > allow also ip register. The following patch is an attempt to do the same > thing, just in the same patterns through arch_enabled attribute. > > Completely untested. > > 2019-02-08 Jakub Jelinek > > PR bootstrap/88714 > * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Add alternative with > q constraint instead of r, enable it only if not TARGET_REALLY_IWMMXT. I've started validations with this patch, I expect the results later today. Thanks > > --- gcc/config/arm/ldrdstrd.md.jj 2019-02-07 17:33:38.841669141 +0100 > +++ gcc/config/arm/ldrdstrd.md 2019-02-08 10:42:56.515325579 +0100 > @@ -157,10 +157,10 @@ (define_peephole2 ; swap the destination > ;; We use gen_operands_ldrd_strd() with a modify argument as false so that > the > ;; operands are not changed. > (define_insn "*arm_ldrd" > - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") > - (match_operand:SI 2 "memory_operand" "m")) > - (set (match_operand:SI 1 "s_register_operand" "=r") > - (match_operand:SI 3 "memory_operand" "m"))])] > + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q,r") > + (match_operand:SI 2 "memory_operand" "m,m")) > + (set (match_operand:SI 1 "s_register_operand" "=q,r") > + (match_operand:SI 3 "memory_operand" "m,m"))])] >"TARGET_LDRD && TARGET_ARM && reload_completed >&& valid_operands_ldrd_strd (operands, true)" >{ > @@ -173,14 +173,17 @@ (define_insn "*arm_ldrd" > (symbol_ref "arm_count_ldrdstrd_insns (operands, true) * 4")) > (set (attr "ce_count") (symbol_ref "get_attr_length (insn) / 4")) > (set_attr "type" "load_8") > - (set_attr "predicable" "yes")] > -) > + (set_attr "predicable" "yes") > + (set (attr "arch_enabled") > + (if_then_else (and (match_test "TARGET_REALLY_IWMMXT") > + (eq_attr "alternative" "0")) > + (const_string "no") (const_string "yes")))]) > > (define_insn "*arm_strd" > - [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") > - (match_operand:SI 0 "s_register_operand" "r")) > - (set (match_operand:SI 3 "memory_operand" "=m") > - (match_operand:SI 1 "s_register_operand" "r"))])] > + [(parallel [(set (match_operand:SI 2 "memory_operand" "=m,m") > + (match_operand:SI 0 "s_register_operand" "q,r")) > + (set (match_operand:SI 3 "memory_operand" "=m,m") > + (match_operand:SI 1 "s_register_operand" "q,r"))])] >"TARGET_LDRD && TARGET_ARM && reload_completed >&& valid_operands_ldrd_strd (operands, false)" >{ > @@ -193,5 +196,8 @@ (define_insn "*arm_strd" > (symbol_ref "arm_count_ldrdstrd_insns (operands, false) * 4")) > (set (attr "ce_count") (symbol_ref "get_attr_length (insn) / 4")) > (set_attr "type" "store_8") > - (set_attr "predicable" "yes")] > -) > + (set_attr "predicable" "yes") > + (set (attr "arch_enabled") > + (if_then_else (and (match_test "TARGET_REALLY_IWMMXT") > + (eq_attr "alternative" "0")) > + (const_string "no") (const_string "yes")))]) > > Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Fri, Feb 08, 2019 at 10:18:03AM +0100, Christophe Lyon wrote: > I'm afaid this patch causes several regressions. Maybe they have > already been fixed post-commit (I have several validations for later > commits still running)? The following patch fixes the single ICE I've tried to reproduce. While iwmmxt.md movdi pattern uses =m, r and =r, m constraints, both arm.md and vfp.md movdi patterns use =m, q and =q, m constraints and thus allow also ip register. The following patch is an attempt to do the same thing, just in the same patterns through arch_enabled attribute. Completely untested. 2019-02-08 Jakub Jelinek PR bootstrap/88714 * config/arm/ldrdstrd.md (*arm_ldrd, *arm_strd): Add alternative with q constraint instead of r, enable it only if not TARGET_REALLY_IWMMXT. --- gcc/config/arm/ldrdstrd.md.jj 2019-02-07 17:33:38.841669141 +0100 +++ gcc/config/arm/ldrdstrd.md 2019-02-08 10:42:56.515325579 +0100 @@ -157,10 +157,10 @@ (define_peephole2 ; swap the destination ;; We use gen_operands_ldrd_strd() with a modify argument as false so that the ;; operands are not changed. (define_insn "*arm_ldrd" - [(parallel [(set (match_operand:SI 0 "s_register_operand" "=r") - (match_operand:SI 2 "memory_operand" "m")) - (set (match_operand:SI 1 "s_register_operand" "=r") - (match_operand:SI 3 "memory_operand" "m"))])] + [(parallel [(set (match_operand:SI 0 "s_register_operand" "=q,r") + (match_operand:SI 2 "memory_operand" "m,m")) + (set (match_operand:SI 1 "s_register_operand" "=q,r") + (match_operand:SI 3 "memory_operand" "m,m"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, true)" { @@ -173,14 +173,17 @@ (define_insn "*arm_ldrd" (symbol_ref "arm_count_ldrdstrd_insns (operands, true) * 4")) (set (attr "ce_count") (symbol_ref "get_attr_length (insn) / 4")) (set_attr "type" "load_8") - (set_attr "predicable" "yes")] -) + (set_attr "predicable" "yes") + (set (attr "arch_enabled") + (if_then_else (and (match_test "TARGET_REALLY_IWMMXT") + (eq_attr "alternative" "0")) + (const_string "no") (const_string "yes")))]) (define_insn "*arm_strd" - [(parallel [(set (match_operand:SI 2 "memory_operand" "=m") - (match_operand:SI 0 "s_register_operand" "r")) - (set (match_operand:SI 3 "memory_operand" "=m") - (match_operand:SI 1 "s_register_operand" "r"))])] + [(parallel [(set (match_operand:SI 2 "memory_operand" "=m,m") + (match_operand:SI 0 "s_register_operand" "q,r")) + (set (match_operand:SI 3 "memory_operand" "=m,m") + (match_operand:SI 1 "s_register_operand" "q,r"))])] "TARGET_LDRD && TARGET_ARM && reload_completed && valid_operands_ldrd_strd (operands, false)" { @@ -193,5 +196,8 @@ (define_insn "*arm_strd" (symbol_ref "arm_count_ldrdstrd_insns (operands, false) * 4")) (set (attr "ce_count") (symbol_ref "get_attr_length (insn) / 4")) (set_attr "type" "store_8") - (set_attr "predicable" "yes")] -) + (set_attr "predicable" "yes") + (set (attr "arch_enabled") + (if_then_else (and (match_test "TARGET_REALLY_IWMMXT") + (eq_attr "alternative" "0")) + (const_string "no") (const_string "yes")))]) Jakub
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
On Tue, 5 Feb 2019 at 15:44, Matthew Malcomson wrote: > > These peepholes match a pair of SImode loads or stores that can be > implemented with a single LDRD or STRD instruction. > When compiling for TARGET_ARM, these peepholes originally created a set > pattern in DI mode to be caught by movdi patterns. > > This approach failed to take into account the possibility that the two > matched insns operated on memory with different aliasing information. > The peepholes lost the aliasing information on one of the insns, which > could then cause the scheduler to make an invalid transformation. > > This patch changes the peepholes so they generate a PARALLEL expression > of the two relevant loads or stores, which means the aliasing > information of both is kept. Such a PARALLEL pattern is what the > peepholes currently produce for TARGET_THUMB2. > > In order to match these new insn patterns, we add two new define_insn's. > These > define_insn's use the same checks as the peepholes to find valid insns. > > Note that the patterns now created by the peepholes for LDRD and STRD > are very similar to those created by the peepholes for LDM and STM. > Many patterns could be matched by the LDM and STM define_insns, which > means we rely on the order the define_insn patterns are defined in the > machine description, with those for LDRD/STRD defined before those for > LDM/STM. > > The difference between the peepholes for LDRD/STRD and those for LDM/STM > are mainly that those for LDRD/STRD have some logic to ensure that the > two registers are consecutive and the first one is even. > > Bootstrapped and regtested on arm-none-linux-gnu. > Demonstrated fix of bug 88714 by bootstrapping on armv7l. > > > gcc/ChangeLog: > > 2019-02-05 Matthew Malcomson > > PR bootstrap/88714 > * config/arm/arm-protos.h (valid_operands_ldrd_strd, > arm_count_ldrdstrd_insns): New declarations. > * config/arm/arm.c (mem_ok_for_ldrd_strd): Remove broken handling of > MINUS. > (valid_operands_ldrd_strd): New function. > (arm_count_ldrdstrd_insns): New function. > * config/arm/ldrdstrd.md: Change peepholes to generate PARALLEL SImode > sets instead of single DImode set and define new insns to match this. > > gcc/testsuite/ChangeLog: > > 2019-02-05 Matthew Malcomson > > * gcc.c-torture/execute/pr88714.c: New test. > * gcc.dg/rtl/arm/ldrd-peepholes.c: New test. > Hi! I'm afaid this patch causes several regressions. Maybe they have already been fixed post-commit (I have several validations for later commits still running)? For the whole picture, see: http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/268644/report-build-info.html Namely there are some ICEs: --target arm-none-linux-gnueabi --with-mode arm --with-cpu cortex-a9 --with-fpu default gcc.c-torture/execute/builtins/memcpy-chk.c compilation, -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) gcc.c-torture/execute/builtins/memmove-chk.c compilation, -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) gcc.c-torture/execute/builtins/mempcpy-chk.c compilation, -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) gcc.c-torture/execute/builtins/memset-chk.c compilation, -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -O2 (internal compiler error) gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions (internal compiler error) gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -O3 -g (internal compiler error) gcc.c-torture/execute/builtins/stpcpy-chk.c compilation, -O2 (internal compiler error) gcc.c-torture/execute/builtins/stpcpy-chk.c compilation, -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) gcc.c-torture/execute/builtins/stpcpy-chk.c compilation, -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) gcc.c-torture/execute/builtins/stpcpy-chk.c compilation, -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions (internal compiler error) gcc.c-torture/execute/builtins/stpcpy-chk.c compilation, -O3 -g (internal compiler error) gcc.c-torture/execute/builtins/strcat-chk.c compilation, -O2 (internal compiler error) gcc.c-torture/execute/builtins/strcat-chk.c compilation, -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) gcc.c-torture/execute/builtins/strcat-chk.c compilation, -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) gcc.c-torture/execute/builtins/strcat-chk.c compil
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
>> > > Please add the PR marker to the testsuite ChangeLog as well. > I've been following this PR a bit from the sidelines, I believe a > substantial amount of code > (and one of the testcases) was written by Jakub, so please add him to > the ChangeLog entries as well. > > This looks ok to me. > Thanks for fixing this and thanks Jakub for the analysis and fixes too! > > Kyrill > Thanks Kyrill, I've committed with the changelog below. gcc/ChangeLog: 2019-02-07 Matthew Malcomson Jakub Jelinek PR bootstrap/88714 * config/arm/arm-protos.h (valid_operands_ldrd_strd, arm_count_ldrdstrd_insns): New declarations. * config/arm/arm.c (mem_ok_for_ldrd_strd): Remove broken handling of MINUS. (valid_operands_ldrd_strd): New function. (arm_count_ldrdstrd_insns): New function. * config/arm/ldrdstrd.md: Change peepholes to generate PARALLEL SImode sets instead of single DImode set and define new insns to match this. gcc/testsuite/ChangeLog: 2019-02-07 Matthew Malcomson Jakub Jelinek PR bootstrap/88714 * gcc.c-torture/execute/pr88714.c: New test. * gcc.dg/rtl/arm/ldrd-peepholes.c: New test.
Re: [Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
Hi Matthew, On 05/02/19 14:44, Matthew Malcomson wrote: These peepholes match a pair of SImode loads or stores that can be implemented with a single LDRD or STRD instruction. When compiling for TARGET_ARM, these peepholes originally created a set pattern in DI mode to be caught by movdi patterns. This approach failed to take into account the possibility that the two matched insns operated on memory with different aliasing information. The peepholes lost the aliasing information on one of the insns, which could then cause the scheduler to make an invalid transformation. This patch changes the peepholes so they generate a PARALLEL expression of the two relevant loads or stores, which means the aliasing information of both is kept. Such a PARALLEL pattern is what the peepholes currently produce for TARGET_THUMB2. In order to match these new insn patterns, we add two new define_insn's. These define_insn's use the same checks as the peepholes to find valid insns. Note that the patterns now created by the peepholes for LDRD and STRD are very similar to those created by the peepholes for LDM and STM. Many patterns could be matched by the LDM and STM define_insns, which means we rely on the order the define_insn patterns are defined in the machine description, with those for LDRD/STRD defined before those for LDM/STM. The difference between the peepholes for LDRD/STRD and those for LDM/STM are mainly that those for LDRD/STRD have some logic to ensure that the two registers are consecutive and the first one is even. Bootstrapped and regtested on arm-none-linux-gnu. Demonstrated fix of bug 88714 by bootstrapping on armv7l. gcc/ChangeLog: 2019-02-05 Matthew Malcomson PR bootstrap/88714 * config/arm/arm-protos.h (valid_operands_ldrd_strd, arm_count_ldrdstrd_insns): New declarations. * config/arm/arm.c (mem_ok_for_ldrd_strd): Remove broken handling of MINUS. (valid_operands_ldrd_strd): New function. (arm_count_ldrdstrd_insns): New function. * config/arm/ldrdstrd.md: Change peepholes to generate PARALLEL SImode sets instead of single DImode set and define new insns to match this. gcc/testsuite/ChangeLog: 2019-02-05 Matthew Malcomson * gcc.c-torture/execute/pr88714.c: New test. * gcc.dg/rtl/arm/ldrd-peepholes.c: New test. Please add the PR marker to the testsuite ChangeLog as well. I've been following this PR a bit from the sidelines, I believe a substantial amount of code (and one of the testcases) was written by Jakub, so please add him to the ChangeLog entries as well. This looks ok to me. Thanks for fixing this and thanks Jakub for the analysis and fixes too! Kyrill ### Attachment also inlined for ease of reply### diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h index 79ede0db174fcce87abe8b4d18893550d4c7e2f6..485bc68a618d6ae4a1640368ccb025fe2c9e1420 100644 --- a/gcc/config/arm/arm-protos.h +++ b/gcc/config/arm/arm-protos.h @@ -125,6 +125,7 @@ extern rtx arm_gen_store_multiple (int *, int, rtx, int, rtx, HOST_WIDE_INT *); extern bool offset_ok_for_ldrd_strd (HOST_WIDE_INT); extern bool operands_ok_ldrd_strd (rtx, rtx, rtx, HOST_WIDE_INT, bool, bool); extern bool gen_operands_ldrd_strd (rtx *, bool, bool, bool); +extern bool valid_operands_ldrd_strd (rtx *, bool); extern int arm_gen_movmemqi (rtx *); extern bool gen_movmem_ldrd_strd (rtx *); extern machine_mode arm_select_cc_mode (RTX_CODE, rtx, rtx); @@ -146,6 +147,7 @@ extern const char *output_mov_long_double_arm_from_arm (rtx *); extern const char *output_move_double (rtx *, bool, int *count); extern const char *output_move_quad (rtx *); extern int arm_count_output_move_double_insns (rtx *); +extern int arm_count_ldrdstrd_insns (rtx *, bool); extern const char *output_move_vfp (rtx *operands); extern const char *output_move_neon (rtx *operands); extern int arm_attr_length_move_neon (rtx_insn *); diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 73cb8df9af1ec9d680091bb8691bcd925a1be1d3..1c336da9e5b0948ef1058c46966364510dc1ca38 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -15556,7 +15556,7 @@ mem_ok_for_ldrd_strd (rtx mem, rtx *base, rtx *offset, HOST_WIDE_INT *align) *base = addr; return true; } - else if (GET_CODE (addr) == PLUS || GET_CODE (addr) == MINUS) + else if (GET_CODE (addr) == PLUS) { *base = XEXP (addr, 0); *offset = XEXP (addr, 1); @@ -15721,7 +15721,7 @@ gen_operands_ldrd_strd (rtx *operands, bool load, } /* Make sure accesses are to consecutive memory locations. */ - if (gap != 4) + if (gap != GET_MODE_SIZE (SImode)) return false; if (!align_ok_ldrd_strd (align[0], offset)) @@ -15802,6 +15802,55 @@ gen_operands_ldrd_strd (rtx *operands, bool load, } +/* Return true if parallel execution of the two word-size accesses provided + could be satisfied with a single LDR
[Patch] [arm] Fix 88714, Arm LDRD/STRD peepholes
These peepholes match a pair of SImode loads or stores that can be implemented with a single LDRD or STRD instruction. When compiling for TARGET_ARM, these peepholes originally created a set pattern in DI mode to be caught by movdi patterns. This approach failed to take into account the possibility that the two matched insns operated on memory with different aliasing information. The peepholes lost the aliasing information on one of the insns, which could then cause the scheduler to make an invalid transformation. This patch changes the peepholes so they generate a PARALLEL expression of the two relevant loads or stores, which means the aliasing information of both is kept. Such a PARALLEL pattern is what the peepholes currently produce for TARGET_THUMB2. In order to match these new insn patterns, we add two new define_insn's. These define_insn's use the same checks as the peepholes to find valid insns. Note that the patterns now created by the peepholes for LDRD and STRD are very similar to those created by the peepholes for LDM and STM. Many patterns could be matched by the LDM and STM define_insns, which means we rely on the order the define_insn patterns are defined in the machine description, with those for LDRD/STRD defined before those for LDM/STM. The difference between the peepholes for LDRD/STRD and those for LDM/STM are mainly that those for LDRD/STRD have some logic to ensure that the two registers are consecutive and the first one is even. Bootstrapped and regtested on arm-none-linux-gnu. Demonstrated fix of bug 88714 by bootstrapping on armv7l. gcc/ChangeLog: 2019-02-05 Matthew Malcomson PR bootstrap/88714 * config/arm/arm-protos.h (valid_operands_ldrd_strd, arm_count_ldrdstrd_insns): New declarations. * config/arm/arm.c (mem_ok_for_ldrd_strd): Remove broken handling of MINUS. (valid_operands_ldrd_strd): New function. (arm_count_ldrdstrd_insns): New function. * config/arm/ldrdstrd.md: Change peepholes to generate PARALLEL SImode sets instead of single DImode set and define new insns to match this. gcc/testsuite/ChangeLog: 2019-02-05 Matthew Malcomson * gcc.c-torture/execute/pr88714.c: New test. * gcc.dg/rtl/arm/ldrd-peepholes.c: New test. ### Attachment also inlined for ease of reply### diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h index 79ede0db174fcce87abe8b4d18893550d4c7e2f6..485bc68a618d6ae4a1640368ccb025fe2c9e1420 100644 --- a/gcc/config/arm/arm-protos.h +++ b/gcc/config/arm/arm-protos.h @@ -125,6 +125,7 @@ extern rtx arm_gen_store_multiple (int *, int, rtx, int, rtx, HOST_WIDE_INT *); extern bool offset_ok_for_ldrd_strd (HOST_WIDE_INT); extern bool operands_ok_ldrd_strd (rtx, rtx, rtx, HOST_WIDE_INT, bool, bool); extern bool gen_operands_ldrd_strd (rtx *, bool, bool, bool); +extern bool valid_operands_ldrd_strd (rtx *, bool); extern int arm_gen_movmemqi (rtx *); extern bool gen_movmem_ldrd_strd (rtx *); extern machine_mode arm_select_cc_mode (RTX_CODE, rtx, rtx); @@ -146,6 +147,7 @@ extern const char *output_mov_long_double_arm_from_arm (rtx *); extern const char *output_move_double (rtx *, bool, int *count); extern const char *output_move_quad (rtx *); extern int arm_count_output_move_double_insns (rtx *); +extern int arm_count_ldrdstrd_insns (rtx *, bool); extern const char *output_move_vfp (rtx *operands); extern const char *output_move_neon (rtx *operands); extern int arm_attr_length_move_neon (rtx_insn *); diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 73cb8df9af1ec9d680091bb8691bcd925a1be1d3..1c336da9e5b0948ef1058c46966364510dc1ca38 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -15556,7 +15556,7 @@ mem_ok_for_ldrd_strd (rtx mem, rtx *base, rtx *offset, HOST_WIDE_INT *align) *base = addr; return true; } - else if (GET_CODE (addr) == PLUS || GET_CODE (addr) == MINUS) + else if (GET_CODE (addr) == PLUS) { *base = XEXP (addr, 0); *offset = XEXP (addr, 1); @@ -15721,7 +15721,7 @@ gen_operands_ldrd_strd (rtx *operands, bool load, } /* Make sure accesses are to consecutive memory locations. */ - if (gap != 4) + if (gap != GET_MODE_SIZE (SImode)) return false; if (!align_ok_ldrd_strd (align[0], offset)) @@ -15802,6 +15802,55 @@ gen_operands_ldrd_strd (rtx *operands, bool load, } +/* Return true if parallel execution of the two word-size accesses provided + could be satisfied with a single LDRD/STRD instruction. Two word-size + accesses are represented by the OPERANDS array, where OPERANDS[0,1] are + register operands and OPERANDS[2,3] are the corresponding memory operands. + */ +bool +valid_operands_ldrd_strd (rtx *operands, bool load) +{ + int nops = 2; + HOST_WIDE_INT offsets[2], offset, align[2]; + rtx base = NULL_RTX; + rtx cur_base, cur_offset; + int i, gap; + + /* Check that the me