Patchwork Patch for 4.7: Avoid subreg'ing VFP D registers in big-endian mode

login
register
mail settings
Submitter Seth LaForge
Date Feb. 15, 2013, 10:40 p.m.
Message ID <CAC_HCCPAzf50DHNwTc90qUD6eMWrCZ2=OuHzpaghQ4gVkb0MyA@mail.gmail.com>
Download mbox | patch
Permalink /patch/220889/
State New
Headers show

Comments

Seth LaForge - Feb. 15, 2013, 10:40 p.m.
gcc 4.7.2 generates incorrect code for big-endian ARM VFP processors
when storing a local double to a packed memory location, as described
in bug 56351.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56351

A fix has been submitted to the trunk by Julian Brown:
http://gcc.gnu.org/git/?p=gcc.git;a=patch;h=4f0e4afcb2a9d345ed7e9c14233fb97d3a3e8d3c

This fix is not included in the latest 4.7 snapshot.  Can we please
include it in the 4.7 branch so that this serious codegen bug is fixed
in 4.7.3?

Unfortunately, it's a not a trivial backport, as it appears that
support for ARM FPA floating point has been removed from gcc since
4.7.  Below is my best attempt at a backport - it works for me, on a
big-endian VFP processor.  I'm not familiar with this code, though -
Julian, can you take a look and confirm whether I'm doing this right?

2013-02-15 Seth LaForge <sethml@google.com>

	* config/arm/arm.h (CANNOT_CHANGE_MODE_CLASS): Avoid subreg'ing
	VFP D registers in big-endian mode.
	Backported from a change by Julian Brown <julian@codesourcery.com>
Ramana Radhakrishnan - Feb. 19, 2013, 10:59 p.m.
On Fri, Feb 15, 2013 at 10:40 PM, Seth LaForge <sethml@google.com> wrote:
> gcc 4.7.2 generates incorrect code for big-endian ARM VFP processors
> when storing a local double to a packed memory location, as described
> in bug 56351.
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56351
>
> A fix has been submitted to the trunk by Julian Brown:
> http://gcc.gnu.org/git/?p=gcc.git;a=patch;h=4f0e4afcb2a9d345ed7e9c14233fb97d3a3e8d3c
>
> This fix is not included in the latest 4.7 snapshot.  Can we please
> include it in the 4.7 branch so that this serious codegen bug is fixed
> in 4.7.3?
>
> Unfortunately, it's a not a trivial backport, as it appears that
> support for ARM FPA floating point has been removed from gcc since
> 4.7.  Below is my best attempt at a backport - it works for me, on a
> big-endian VFP processor.  I'm not familiar with this code, though -
> Julian, can you take a look and confirm whether I'm doing this right?
>
> 2013-02-15 Seth LaForge <sethml@google.com>
>
>         * config/arm/arm.h (CANNOT_CHANGE_MODE_CLASS): Avoid subreg'ing
>         VFP D registers in big-endian mode.
>         Backported from a change by Julian Brown <julian@codesourcery.com>

This is not the correct form of a changelog entry . Please see other
examples of Backports in the Changelog file for the 4.7 branch.

Instead please use something like this.

<DATE>  <yourname>  <youremailid>

<TAB>Backport
<TAB>2012-10-22  Julian Brown  <julian@codesourcery.com>
	  * config/arm/arm.h (CANNOT_CHANGE_MODE_CLASS): Avoid subreg'ing
	  VFP D registers in big-endian mode.

Ok to backport provided no regressions  when running the testsuite in
big endian mode.

>
> diff -urp gcc-4.7.2/gcc/config/arm/arm.h gcc-4.7.2.new/gcc/config/arm/arm.h
> --- gcc-4.7.2/gcc/config/arm/arm.h      2012-01-09 20:14:09.000000000 -0800
> +++ gcc-4.7.2.new/gcc/config/arm/arm.h  2013-02-15 14:03:09.113531299 -0800
> @@ -1123,11 +1123,18 @@ enum reg_class
>  /* FPA registers can't do subreg as all values are reformatted to internal
>     precision.  In VFPv1, VFP registers could only be accessed in the mode
>     they were set, so subregs would be invalid there too.  However, we don't
> -   support VFPv1 at the moment, and the restriction was lifted in VFPv2.  */
> +   support VFPv1 at the moment, and the restriction was lifted in VFPv2.
> +   In big-endian mode, modes greater than word size (i.e. DFmode) are stored in
> +   VFP registers in little-endian order.  We can't describe that accurately to
> +   GCC, so avoid taking subregs of such values.  */
>  #define CANNOT_CHANGE_MODE_CLASS(FROM, TO, CLASS)              \
> -  (GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO)                  \
> -   ? reg_classes_intersect_p (FPA_REGS, (CLASS))               \
> -   : 0)
> +  (TARGET_VFP                                                  \
> +  ? TARGET_BIG_END                                             \
> +    && (GET_MODE_SIZE (FROM) > UNITS_PER_WORD                  \
> +       || GET_MODE_SIZE (TO) > UNITS_PER_WORD)                 \
> +    && reg_classes_intersect_p (VFP_REGS, (CLASS))             \
> +  : GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO)                 \
> +    && reg_classes_intersect_p (FPA_REGS, (CLASS)))

Watch out for alignment of the \'s though it might be your mailer
munging things rather than anything else. Can you please make sure
that the \'s are aligned to the same column please ? This is why I
prefer to get git to generate out patches using git-format-patch and
attach them as text files to all my mails otherwise it seems to mess
things up like this.

Thanks,
Ramana
Seth LaForge - Feb. 20, 2013, 11:40 p.m.
On Tue, Feb 19, 2013 at 2:59 PM, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
> This is not the correct form of a changelog entry.

Sorry - another attempt below.

> Ok to backport provided no regressions  when running the testsuite in
> big endian mode.

Sorry to say I'm not sure how to run the testsuite, given that I'm
cross-compiling for a wacky TI automotive microcontroller.  I'm out of
town for the next few days, but I can try to run it under qemu next
week.

> Watch out for alignment of the \'s though it might be your mailer
> munging things rather than anything else. Can you please make sure
> that the \'s are aligned to the same column please ? This is why I
> prefer to get git to generate out patches using git-format-patch and
> attach them as text files to all my mails otherwise it seems to mess
> things up like this.

They're aligned for me, and I successfully applied my patch by copying
it out of my mail, so I blame your mail client.  :)  In any case, git
patch attached.

Seth

Hopefully correct changelog entry:

2013-02-20 Seth LaForge <sethml@google.com>

	Backport
	2012-10-22  Julian Brown  <julian@codesourcery.com>
	* config/arm/arm.h (CANNOT_CHANGE_MODE_CLASS): Avoid subreg'ing
	VFP D registers in big-endian mode.
Eric Botcazou - March 14, 2013, 6:54 p.m.
> 2013-02-20 Seth LaForge <sethml@google.com>
> 
> 	Backport
> 	2012-10-22  Julian Brown  <julian@codesourcery.com>
> 	* config/arm/arm.h (CANNOT_CHANGE_MODE_CLASS): Avoid subreg'ing
> 	VFP D registers in big-endian mode.

Applied to the 4.7 branch after QEMU-testing for arm-eabi in big-endian mode.
It also fixes a fair number of execution failures in the testsuite.

Patch

diff -urp gcc-4.7.2/gcc/config/arm/arm.h gcc-4.7.2.new/gcc/config/arm/arm.h
--- gcc-4.7.2/gcc/config/arm/arm.h	2012-01-09 20:14:09.000000000 -0800
+++ gcc-4.7.2.new/gcc/config/arm/arm.h	2013-02-15 14:03:09.113531299 -0800
@@ -1123,11 +1123,18 @@  enum reg_class
 /* FPA registers can't do subreg as all values are reformatted to internal
    precision.  In VFPv1, VFP registers could only be accessed in the mode
    they were set, so subregs would be invalid there too.  However, we don't
-   support VFPv1 at the moment, and the restriction was lifted in VFPv2.  */
+   support VFPv1 at the moment, and the restriction was lifted in VFPv2.
+   In big-endian mode, modes greater than word size (i.e. DFmode) are stored in
+   VFP registers in little-endian order.  We can't describe that accurately to
+   GCC, so avoid taking subregs of such values.  */
 #define CANNOT_CHANGE_MODE_CLASS(FROM, TO, CLASS)		\
-  (GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO)			\
-   ? reg_classes_intersect_p (FPA_REGS, (CLASS))		\
-   : 0)
+  (TARGET_VFP							\
+  ? TARGET_BIG_END						\
+    && (GET_MODE_SIZE (FROM) > UNITS_PER_WORD			\
+       || GET_MODE_SIZE (TO) > UNITS_PER_WORD)			\
+    && reg_classes_intersect_p (VFP_REGS, (CLASS))		\
+  : GET_MODE_SIZE (FROM) != GET_MODE_SIZE (TO)			\
+    && reg_classes_intersect_p (FPA_REGS, (CLASS)))

 /* The class value for index registers, and the one for base regs.  */
 #define INDEX_REG_CLASS  (TARGET_THUMB1 ? LO_REGS : GENERAL_REGS)