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 =
+ // `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 =
+// `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
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
[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
