| Message ID | 87lh6k8tf8.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 16 February 2016 at 18:19, Nick Clifton <nickc@redhat.com> wrote: > Hi Richard, Hi Ramana, > > The ARM backend has some problems compiling for the old ARMv3 > architecture. See: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62254 > > for an example of this. v3 is very old now, and I am not sure how > much interest there is in continuing to support it, but I am trying to > help reduce the gcc priority bug list, so here goes... > > The attached patch fixes the problem, albeit not in a very subtle way. > The problem is that arm_reload_[out|in]_hi is being called for a > register->register move because the v3 architecture does not support > 16-bit register moves. Rather than trace the problem back to the real > source and fix it, I chose to just allow the reload functions to > generate an SImode register move instead. Probably not the best > solution, but it appears to work. > > The attached patch also includes the test cases derived from PR 62254 > and PR 69610 (which is a duplicate of PR 62254). Including all three > tests might be overkill, but it seemed like a good idea to be a little > bit paranoid, just in case. > > Whilst testing the patch I also discovered that interworking is > enabled by default, which is a problem for v3 code generation, so I > added a fix to (silently) disable interworking if the target > architecture does not support Thumb instructions. > > Any comments or criticisms before I apply the patch ? Hi Nick, Could you modify your new testcases, such that they call check_effective_target_arm_arm_ok ? Since you force -marm, you need to make sure that the target cpu actually supports it. I'm just realizing that we currently generate arm_arch_vX_ok for X >=4 only. Maybe you should also add v3? Thanks Christophe. > > Cheers > Nick > > gcc/ChangeLog > 2016-02-16 Nick Clifton <nickc@redhat.com> > > PR target/62554 > PR target/69610 > * config/arm/arm.c (arm_option_override_internal): Disable > interworking if the target does not support thumb instructions. > (arm_reload_in_hi): Handle the case where a register to register > move needs reloading because there is no simple pattern to handle > it. > (arm_reload_out_hi): Likewise. > > gcc/testsuite/ChangeLog > 2016-02-16 Nick Clifton <nickc@redhat.com> > > PR target/62554 > PR target/69610 > * gcc.target/arm/pr62554.c: New test. > * gcc.target/arm/pr69610-1.c: New test. > * gcc.target/arm/pr69610-2.c: New test. >
Hi Christophe, > Could you modify your new testcases, such that they call > check_effective_target_arm_arm_ok ? Good idea - done. > I'm just realizing that we currently generate arm_arch_vX_ok > for X >=4 only. Maybe you should also add v3? Possibly. I am not at all sure how important v3 support is any more (or support for any ARM architecture prior to v4t). Do you know of anyone who still needs it ? Cheers Nick
On 18 February 2016 at 14:20, Nick Clifton <nickc@redhat.com> wrote: > Hi Christophe, > >> Could you modify your new testcases, such that they call >> check_effective_target_arm_arm_ok ? > > Good idea - done. > >> I'm just realizing that we currently generate arm_arch_vX_ok >> for X >=4 only. Maybe you should also add v3? > > Possibly. I am not at all sure how important v3 support is any more > (or support for any ARM architecture prior to v4t). Do you know of > anyone who still needs it ? > I am not aware of any such user, but the ARM maintainers would know better for sure. FWIW, the original bug report came from the kernel team, when building all the existing configurations. So it seems the Linux kernel still wants to support v3. > Cheers > Nick
On 18/02/16 13:28, Christophe Lyon wrote: > On 18 February 2016 at 14:20, Nick Clifton <nickc@redhat.com> wrote: >> Hi Christophe, >> >>> Could you modify your new testcases, such that they call >>> check_effective_target_arm_arm_ok ? >> Good idea - done. >> >>> I'm just realizing that we currently generate arm_arch_vX_ok >>> for X >=4 only. Maybe you should also add v3? >> Possibly. I am not at all sure how important v3 support is any more >> (or support for any ARM architecture prior to v4t). Do you know of >> anyone who still needs it ? >> > I am not aware of any such user, but the ARM maintainers would know > better for sure. > > FWIW, the original bug report came from the kernel team, when building > all the existing > configurations. So it seems the Linux kernel still wants to support v3. Judging by the Linaro issue linked to the BZ: https://bugs.launchpad.net/gcc-linaro/+bug/1307197 the armv3 build was part of testing with random build configurations, so I don't think that's an indication that real kernels care about this. Kyrill > >> Cheers >> Nick
Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c (revision 233443) +++ gcc/config/arm/arm.c (working copy) @@ -2874,6 +2874,14 @@ { arm_override_options_after_change_1 (opts); + if (TARGET_INTERWORK && !ARM_FSET_HAS_CPU1 (insn_flags, FL_THUMB)) + { + /* The default is to enable interworking, so this warning message would + be confusing to users who have just compiled with, eg, -march=armv3. */ + /* warning (0, "ignoring -minterwork because target CPU does not support THUMB"); */ + opts->x_target_flags &= ~MASK_INTERWORK; + } + if (TARGET_THUMB_P (opts->x_target_flags) && !(ARM_FSET_HAS_CPU1 (insn_flags, FL_THUMB))) { @@ -15440,6 +15448,17 @@ else /* The slot is out of range, or was dressed up in a SUBREG. */ base = reg_equiv_address (REGNO (ref)); + + /* PR 62554: If there is no equivalent memory location then just move + the value as an SImode register move. This happens when the target + architecure variant does not have an HImode register move. */ + if (base == NULL) + { + gcc_assert (REG_P (operands[0])); + emit_insn (gen_movsi (gen_rtx_SUBREG (SImode, operands[0], 0), + gen_rtx_SUBREG (SImode, ref, 0))); + return; + } } else base = find_replacement (&XEXP (ref, 0)); @@ -15557,6 +15576,17 @@ else /* The slot is out of range, or was dressed up in a SUBREG. */ base = reg_equiv_address (REGNO (ref)); + + /* PR 62554: If there is no equivalent memory location then just move + the value as an SImode register move. This happens when the target + architecure variant does not have an HImode register move. */ + if (base == NULL) + { + gcc_assert (REG_P (outval)); + emit_insn (gen_movsi (gen_rtx_SUBREG (SImode, ref, 0), + gen_rtx_SUBREG (SImode, outval, 0))); + return; + } } else base = find_replacement (&XEXP (ref, 0)); @@ -19619,6 +19649,7 @@ break; case ARM_FT_INTERWORKED: + gcc_assert (arm_arch5 || arm_arch4t); sprintf (instr, "bx%s\t%%|lr", conditional); break; --- /dev/null 2016-02-16 08:27:18.513962320 +0000 +++ gcc/testsuite/gcc.target/arm/pr62254.c 2016-02-16 16:47:30.479378118 +0000 @@ -0,0 +1,50 @@ +/* Check that pre ARMv4 compilation still works. */ +/* { dg-do compile } */ +/* { dg-options "-marm -march=armv3 -O" } */ + +typedef struct +{ + char bits; + short val; +} code; + +union uu +{ + short us; + char b[2]; +}; + +int a, b, c, f, g, h; +code *d; + +code e; + +int +fn1 (void) +{ + char i; + do + if (e.bits) + { + dodist: + f = c; + if (e.bits & 6) + { + ++i; + if (g) + do + { + union uu j; + j.b[1] = a; + h = j.us; + } + while (fn1); + } + else + { + e = d[b]; + goto dodist; + } + } + while (i); +} --- /dev/null 2016-02-16 08:27:18.513962320 +0000 +++ gcc/testsuite/gcc.target/arm/pr69610-1.c 2016-02-16 16:51:48.987779288 +0000 @@ -0,0 +1,13 @@ +/* Check that pre ARMv4 compilation still works. */ +/* { dg-do compile } */ +/* { dg-options "-marm -march=armv3 -ftree-ter" } */ + +typedef unsigned short v16u16 __attribute__ ((vector_size (16))); +typedef unsigned int v16u32 __attribute__ ((vector_size (16))); + +unsigned short +foo (v16u16 v16u16_1, v16u32 v16u32_1) +{ + v16u16_1 += (v16u16) v16u32_1; + return v16u16_1[5] + v16u32_1[1]; +} --- /dev/null 2016-02-16 08:27:18.513962320 +0000 +++ gcc/testsuite/gcc.target/arm/pr69610-2.c 2016-02-16 16:51:27.119660758 +0000 @@ -0,0 +1,32 @@ +/* Check that pre ARMv4 compilation still works. */ +/* { dg-do compile } */ +/* { dg-options "-marm -march=armv3 -O2 -fno-forward-propagate" } */ + +typedef short v16u16 __attribute__ ((vector_size (16))); +typedef unsigned v16u32 __attribute__ ((vector_size (16))); +typedef long long v16u64 __attribute__ ((vector_size (16))); + +unsigned +foo + (int + u16_0, + unsigned + u32_0, + int + u64_0, + int + u16_1, + unsigned + u64_1, + v16u16 + v16u16_0, + v16u32 + v16u32_0, + v16u64 v16u64_0, v16u16 v16u16_1, v16u32 v16u32_1, v16u64 v16u64_1) +{ + v16u16_1[3] -= v16u32_0[0]; + v16u16_0 -= (v16u16) v16u32_0; + return u16_0 + u32_0 + u64_0 + u16_1 + + v16u16_0[0] + v16u16_0[2] + v16u16_0[3] + v16u16_0[4] + v16u16_0[5] + v16u32_0[0] + v16u32_0[1] + v16u32_0[3] + v16u64_0[1] + + v16u16_1[2] + v16u16_1[3] + v16u16_1[5] + v16u16_1[7] + v16u32_1[0] + v16u32_1[3] + v16u64_1[0] + v16u64_1[1]; +}
Hi Richard, Hi Ramana, The ARM backend has some problems compiling for the old ARMv3 architecture. See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62254 for an example of this. v3 is very old now, and I am not sure how much interest there is in continuing to support it, but I am trying to help reduce the gcc priority bug list, so here goes... The attached patch fixes the problem, albeit not in a very subtle way. The problem is that arm_reload_[out|in]_hi is being called for a register->register move because the v3 architecture does not support 16-bit register moves. Rather than trace the problem back to the real source and fix it, I chose to just allow the reload functions to generate an SImode register move instead. Probably not the best solution, but it appears to work. The attached patch also includes the test cases derived from PR 62254 and PR 69610 (which is a duplicate of PR 62254). Including all three tests might be overkill, but it seemed like a good idea to be a little bit paranoid, just in case. Whilst testing the patch I also discovered that interworking is enabled by default, which is a problem for v3 code generation, so I added a fix to (silently) disable interworking if the target architecture does not support Thumb instructions. Any comments or criticisms before I apply the patch ? Cheers Nick gcc/ChangeLog 2016-02-16 Nick Clifton <nickc@redhat.com> PR target/62554 PR target/69610 * config/arm/arm.c (arm_option_override_internal): Disable interworking if the target does not support thumb instructions. (arm_reload_in_hi): Handle the case where a register to register move needs reloading because there is no simple pattern to handle it. (arm_reload_out_hi): Likewise. gcc/testsuite/ChangeLog 2016-02-16 Nick Clifton <nickc@redhat.com> PR target/62554 PR target/69610 * gcc.target/arm/pr62554.c: New test. * gcc.target/arm/pr69610-1.c: New test. * gcc.target/arm/pr69610-2.c: New test.