| Message ID | 583416A0.9010402@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 22, 2016 at 9:57 AM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi all, > > This PR is an ICE while bootstrapping GCC with Cortex-A8 tuning, which we > also get from the default ARMv7-A tuning. > The ldrd/strd peepholes were recently made more aggressive and in this case > they transform: > (insn 13 33 40 2 (set (mem/c:SI (plus:SI (reg/f:SI 11 fp) > (const_int -28 [0xffffffffffffffe4])) [3 d.num_comps+0 S4 > A64]) > (reg:SI 12 ip [orig:117 _20 ] [117])) "cp-demangle.c":32 632 > {*arm_movsi_vfp} > (expr_list:REG_DEAD (reg:SI 12 ip [orig:117 _20 ] [117]) > (nil))) > (insn 40 13 39 2 (set (mem/f/c:SI (plus:SI (reg/f:SI 11 fp) > (const_int -24 [0xffffffffffffffe8])) [2 d.subs+0 S4 A32]) > (reg/f:SI 13 sp)) "cp-demangle.c":51 632 {*arm_movsi_vfp} > (nil)) > > into: > (insn 68 33 39 2 (set (mem/c:DI (plus:SI (reg/f:SI 11 fp) > (const_int -28 [0xffffffffffffffe4])) [3 d.num_comps+0 S8 > A64]) > (reg:DI 12 ip)) "cp-demangle.c":51 -1 > (nil)) > > This is okay, but the *movdi_vfp_cortexa8 pattern doesn't deal with the IP > being the source > of the store. The reason is that when the LDRD/STRD peepholes and machinery > was introduced back in r197530 > it created the 'q' constraint which should be used for the register operands > of the DImode stores and loads > ('q' means CORE_REGS when LDRD/STRD is enabled in ARM mode and GENERAL_REGS > otherwise). That revision > updated the movdi_vfp pattern to use it in alternatives 4,5,6 but neglected > to udpate the Cortex-A8-specific > pattern. This is a sign that we should perhaps get rid of this special-cased > pattern at some point, but for now > this simple patch updates the appropriate alternatives to use the 'q' > constraint so that output_move_double > can output the correct LDRD/STRD instruction. > > Bootstrapped on arm-none-linux-gnueabihf with --with-arch=armv7-a that > exercises this code (bootstrap currently fails > without this patch) and tested with /-mtune=cortex-a8. > > Ok for trunk? Ok. Ramana > > Thanks, > Kyrill > > 2016-11-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/78439 > * config/arm/vfp.md (*movdi_vfp_cortexa8): Use 'q' constraints for the > register operand in alternatives 4,5,6. > > 2016-11-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/78439 > * gcc.c-torture/compile/pr78439.c: New test.
On Tue, Nov 22, 2016 at 9:57 AM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi all, > > This PR is an ICE while bootstrapping GCC with Cortex-A8 tuning, which we > also get from the default ARMv7-A tuning. > The ldrd/strd peepholes were recently made more aggressive and in this case > they transform: > (insn 13 33 40 2 (set (mem/c:SI (plus:SI (reg/f:SI 11 fp) > (const_int -28 [0xffffffffffffffe4])) [3 d.num_comps+0 S4 > A64]) > (reg:SI 12 ip [orig:117 _20 ] [117])) "cp-demangle.c":32 632 > {*arm_movsi_vfp} > (expr_list:REG_DEAD (reg:SI 12 ip [orig:117 _20 ] [117]) > (nil))) > (insn 40 13 39 2 (set (mem/f/c:SI (plus:SI (reg/f:SI 11 fp) > (const_int -24 [0xffffffffffffffe8])) [2 d.subs+0 S4 A32]) > (reg/f:SI 13 sp)) "cp-demangle.c":51 632 {*arm_movsi_vfp} > (nil)) > > into: > (insn 68 33 39 2 (set (mem/c:DI (plus:SI (reg/f:SI 11 fp) > (const_int -28 [0xffffffffffffffe4])) [3 d.num_comps+0 S8 > A64]) > (reg:DI 12 ip)) "cp-demangle.c":51 -1 > (nil)) > > This is okay, but the *movdi_vfp_cortexa8 pattern doesn't deal with the IP > being the source > of the store. The reason is that when the LDRD/STRD peepholes and machinery > was introduced back in r197530 > it created the 'q' constraint which should be used for the register operands > of the DImode stores and loads > ('q' means CORE_REGS when LDRD/STRD is enabled in ARM mode and GENERAL_REGS > otherwise). That revision > updated the movdi_vfp pattern to use it in alternatives 4,5,6 but neglected > to udpate the Cortex-A8-specific > pattern. This is a sign that we should perhaps get rid of this special-cased > pattern at some point, but for now I would expect any patch that does this "i.e. remove the pattern" to be tested to see the impact of the difference in constraints. AFAIR the pattern was added to distinguish between Neon for DImode operations and non-Neon for DImode variations many moons ago. So please do the archeology and measurements ( look at output from crafty for a variety of options and a variety of cores before cleaning all this up). Ramana > this simple patch updates the appropriate alternatives to use the 'q' > constraint so that output_move_double > can output the correct LDRD/STRD instruction. > > Bootstrapped on arm-none-linux-gnueabihf with --with-arch=armv7-a that > exercises this code (bootstrap currently fails > without this patch) and tested with /-mtune=cortex-a8. > > Ok for trunk? > > Thanks, > Kyrill > > 2016-11-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/78439 > * config/arm/vfp.md (*movdi_vfp_cortexa8): Use 'q' constraints for the > register operand in alternatives 4,5,6. > > 2016-11-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/78439 > * gcc.c-torture/compile/pr78439.c: New test.
commit 600526ea992fa58f87e6b0b4f821f4a2dfd0fa7a
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Mon Nov 21 12:00:20 2016 +0000
[ARM] PR target/78439: Update movdi constraints for Cortex-A8 tuning to handled LDRD/STRD
diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
index 2051f10..ce56e16 100644
--- a/gcc/config/arm/vfp.md
+++ b/gcc/config/arm/vfp.md
@@ -355,8 +355,8 @@ (define_insn "*movdi_vfp"
)
(define_insn "*movdi_vfp_cortexa8"
- [(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,Uvi,w"))]
+ [(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,Uvi,w"))]
"TARGET_32BIT && TARGET_HARD_FLOAT && arm_tune == TARGET_CPU_cortexa8
&& ( register_operand (operands[0], DImode)
|| register_operand (operands[1], DImode))
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr78439.c b/gcc/testsuite/gcc.c-torture/compile/pr78439.c
new file mode 100644
index 0000000..a8af86b
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr78439.c
@@ -0,0 +1,56 @@
+/* PR target/78439. */
+
+enum demangle_component_type
+{
+ DEMANGLE_COMPONENT_THROW_SPEC
+};
+struct demangle_component
+{
+ enum demangle_component_type type;
+ struct
+ {
+ struct
+ {
+ struct demangle_component *left;
+ struct demangle_component *right;
+ };
+ };
+};
+
+int a, b;
+
+struct d_info
+{
+ struct demangle_component *comps;
+ int next_comp;
+ int num_comps;
+ struct demangle_component *subs;
+ int num_subs;
+ int is_conversion;
+};
+
+void
+fn1 (int p1, struct d_info *p2)
+{
+ p2->num_comps = 2 * p1;
+ p2->next_comp = p2->num_subs = p1;
+ p2->is_conversion = 0;
+}
+
+int fn3 (int *);
+void fn4 (struct d_info *, int);
+
+void
+fn2 ()
+{
+ int c;
+ struct d_info d;
+ b = 0;
+ c = fn3 (&a);
+ fn1 (c, &d);
+ struct demangle_component e[d.num_comps];
+ struct demangle_component *f[d.num_subs];
+ d.comps = e;
+ d.subs = (struct demangle_component *) f;
+ fn4 (&d, 1);
+}
Hi all, This PR is an ICE while bootstrapping GCC with Cortex-A8 tuning, which we also get from the default ARMv7-A tuning. The ldrd/strd peepholes were recently made more aggressive and in this case they transform: (insn 13 33 40 2 (set (mem/c:SI (plus:SI (reg/f:SI 11 fp) (const_int -28 [0xffffffffffffffe4])) [3 d.num_comps+0 S4 A64]) (reg:SI 12 ip [orig:117 _20 ] [117])) "cp-demangle.c":32 632 {*arm_movsi_vfp} (expr_list:REG_DEAD (reg:SI 12 ip [orig:117 _20 ] [117]) (nil))) (insn 40 13 39 2 (set (mem/f/c:SI (plus:SI (reg/f:SI 11 fp) (const_int -24 [0xffffffffffffffe8])) [2 d.subs+0 S4 A32]) (reg/f:SI 13 sp)) "cp-demangle.c":51 632 {*arm_movsi_vfp} (nil)) into: (insn 68 33 39 2 (set (mem/c:DI (plus:SI (reg/f:SI 11 fp) (const_int -28 [0xffffffffffffffe4])) [3 d.num_comps+0 S8 A64]) (reg:DI 12 ip)) "cp-demangle.c":51 -1 (nil)) This is okay, but the *movdi_vfp_cortexa8 pattern doesn't deal with the IP being the source of the store. The reason is that when the LDRD/STRD peepholes and machinery was introduced back in r197530 it created the 'q' constraint which should be used for the register operands of the DImode stores and loads ('q' means CORE_REGS when LDRD/STRD is enabled in ARM mode and GENERAL_REGS otherwise). That revision updated the movdi_vfp pattern to use it in alternatives 4,5,6 but neglected to udpate the Cortex-A8-specific pattern. This is a sign that we should perhaps get rid of this special-cased pattern at some point, but for now this simple patch updates the appropriate alternatives to use the 'q' constraint so that output_move_double can output the correct LDRD/STRD instruction. Bootstrapped on arm-none-linux-gnueabihf with --with-arch=armv7-a that exercises this code (bootstrap currently fails without this patch) and tested with /-mtune=cortex-a8. Ok for trunk? Thanks, Kyrill 2016-11-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/78439 * config/arm/vfp.md (*movdi_vfp_cortexa8): Use 'q' constraints for the register operand in alternatives 4,5,6. 2016-11-22 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/78439 * gcc.c-torture/compile/pr78439.c: New test.