Patchwork Power/GCC: Fix e500 vs non-e500 register save slot issue

login
register
mail settings
Submitter Maciej W. Rozycki
Date Sept. 1, 2014, 2:22 p.m.
Message ID <alpine.DEB.1.10.1409011515330.2958@tp.orcam.me.uk>
Download mbox | patch
Permalink /patch/384882/
State New
Headers show

Comments

Maciej W. Rozycki - Sept. 1, 2014, 2:22 p.m.
Hi,

 This fixes an issue with the mode used for register save slots on the 
stack where e500 processor support is enabled along non-e500 multilibs.  
In that case the HARD_REGNO_CALLER_SAVE_MODE macro definition from 
gcc/config/rs6000/e500.h overrides one in gcc/config/rs6000/rs6000.h even 
for non-e500 multilibs.  I think the ABI for a given multilib must not 
change with other multilibs being enabled or disabled.

 I have therefore rewritten the generic macro to take both e500 and 
non-e500 cases into account, following the preexisting case of 
TARGET_DF_SPE -- there's no run-time performance hit for purely non-e500 
targets as TARGET_E500_DOUBLE then expands to 0 and the extra e500 support 
code is optimised away.  The change doesn't make the TARGET_VSX case check 
for TARGET_E500_DOUBLE being clear, as the two are mutually exclusive and 
guarded by CHECK_E500_OPTIONS already.

 This fixes:

FAIL: gcc.target/powerpc/pr47862.c scan-assembler-not stfd

failures on non-e500 multilibs.

 Regression-tested with the following powerpc-gnu-linux multilibs:

-mcpu=603e
-mcpu=603e -msoft-float
-mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe
-mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe
-mcpu=7400 -maltivec -mabi=altivec
-mcpu=e6500 -maltivec -mabi=altivec
-mcpu=e5500 -m64
-mcpu=e6500 -m64 -maltivec -mabi=altivec

 OK to apply?

2014-09-01  Maciej W. Rozycki  <macro@codesourcery.com>

	gcc/
	* config/rs6000/e500.h (HARD_REGNO_CALLER_SAVE_MODE): Remove
	macro.
	* config/rs6000/rs6000.h (HARD_REGNO_CALLER_SAVE_MODE): Handle
	TARGET_E500_DOUBLE case here.

  Maciej

gcc-power-linux-e500-hard-regno-caller-save-mode.diff
Maciej W. Rozycki - Sept. 9, 2014, 10:06 p.m.
On Mon, 1 Sep 2014, Maciej W. Rozycki wrote:

>  This fixes an issue with the mode used for register save slots on the 
> stack where e500 processor support is enabled along non-e500 multilibs.  
> In that case the HARD_REGNO_CALLER_SAVE_MODE macro definition from 
> gcc/config/rs6000/e500.h overrides one in gcc/config/rs6000/rs6000.h even 
> for non-e500 multilibs.  I think the ABI for a given multilib must not 
> change with other multilibs being enabled or disabled.
> 
>  I have therefore rewritten the generic macro to take both e500 and 
> non-e500 cases into account, following the preexisting case of 
> TARGET_DF_SPE -- there's no run-time performance hit for purely non-e500 
> targets as TARGET_E500_DOUBLE then expands to 0 and the extra e500 support 
> code is optimised away.  The change doesn't make the TARGET_VSX case check 
> for TARGET_E500_DOUBLE being clear, as the two are mutually exclusive and 
> guarded by CHECK_E500_OPTIONS already.
> 
>  This fixes:
> 
> FAIL: gcc.target/powerpc/pr47862.c scan-assembler-not stfd
> 
> failures on non-e500 multilibs.
> 
>  Regression-tested with the following powerpc-gnu-linux multilibs:
> 
> -mcpu=603e
> -mcpu=603e -msoft-float
> -mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe
> -mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe
> -mcpu=7400 -maltivec -mabi=altivec
> -mcpu=e6500 -maltivec -mabi=altivec
> -mcpu=e5500 -m64
> -mcpu=e6500 -m64 -maltivec -mabi=altivec
> 
>  OK to apply?
> 
> 2014-09-01  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gcc/
> 	* config/rs6000/e500.h (HARD_REGNO_CALLER_SAVE_MODE): Remove
> 	macro.
> 	* config/rs6000/rs6000.h (HARD_REGNO_CALLER_SAVE_MODE): Handle
> 	TARGET_E500_DOUBLE case here.

 Ping!

  Maciej
Maciej W. Rozycki - Sept. 16, 2014, 10:38 p.m.
David,

 This patch:

https://gcc.gnu.org/ml/gcc-patches/2014-09/msg00051.html

is still waiting, please review.

 Thanks,

  Maciej
David Edelsohn - Sept. 24, 2014, 6:31 p.m.
On Mon, Sep 1, 2014 at 10:22 AM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
> Hi,
>
>  This fixes an issue with the mode used for register save slots on the
> stack where e500 processor support is enabled along non-e500 multilibs.
> In that case the HARD_REGNO_CALLER_SAVE_MODE macro definition from
> gcc/config/rs6000/e500.h overrides one in gcc/config/rs6000/rs6000.h even
> for non-e500 multilibs.  I think the ABI for a given multilib must not
> change with other multilibs being enabled or disabled.
>
>  I have therefore rewritten the generic macro to take both e500 and
> non-e500 cases into account, following the preexisting case of
> TARGET_DF_SPE -- there's no run-time performance hit for purely non-e500
> targets as TARGET_E500_DOUBLE then expands to 0 and the extra e500 support
> code is optimised away.  The change doesn't make the TARGET_VSX case check
> for TARGET_E500_DOUBLE being clear, as the two are mutually exclusive and
> guarded by CHECK_E500_OPTIONS already.
>
>  This fixes:
>
> FAIL: gcc.target/powerpc/pr47862.c scan-assembler-not stfd
>
> failures on non-e500 multilibs.
>
>  Regression-tested with the following powerpc-gnu-linux multilibs:
>
> -mcpu=603e
> -mcpu=603e -msoft-float
> -mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe
> -mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe
> -mcpu=7400 -maltivec -mabi=altivec
> -mcpu=e6500 -maltivec -mabi=altivec
> -mcpu=e5500 -m64
> -mcpu=e6500 -m64 -maltivec -mabi=altivec
>
>  OK to apply?
>
> 2014-09-01  Maciej W. Rozycki  <macro@codesourcery.com>
>
>         gcc/
>         * config/rs6000/e500.h (HARD_REGNO_CALLER_SAVE_MODE): Remove
>         macro.
>         * config/rs6000/rs6000.h (HARD_REGNO_CALLER_SAVE_MODE): Handle
>         TARGET_E500_DOUBLE case here.

This patch is okay.  The repeated testing of E500 seems like it could
have been refactored.  The macro is becoming a little overly
complicated as a CASE statement.

Are you avoiding the special cases for TFmode and TDmode on e500 for a
specific reason or simply matching current behavior?

Thanks, David
Joseph S. Myers - Sept. 24, 2014, 8:35 p.m.
On Wed, 24 Sep 2014, David Edelsohn wrote:

> > 2014-09-01  Maciej W. Rozycki  <macro@codesourcery.com>
> >
> >         gcc/
> >         * config/rs6000/e500.h (HARD_REGNO_CALLER_SAVE_MODE): Remove
> >         macro.
> >         * config/rs6000/rs6000.h (HARD_REGNO_CALLER_SAVE_MODE): Handle
> >         TARGET_E500_DOUBLE case here.
> 
> This patch is okay.  The repeated testing of E500 seems like it could
> have been refactored.  The macro is becoming a little overly
> complicated as a CASE statement.
> 
> Are you avoiding the special cases for TFmode and TDmode on e500 for a
> specific reason or simply matching current behavior?

I don't know what's right in the context of the present patch, but the 
general principle for e500 is that TDmode is much like TImode and DDmode 
is much like DImode, but TFmode is much like two of DFmode; that was what 
I concluded when making DFP work for e500 
<https://gcc.gnu.org/ml/gcc-patches/2008-06/msg00270.html>.
Maciej W. Rozycki - Oct. 3, 2014, 8:21 p.m.
On Wed, 24 Sep 2014, Joseph S. Myers wrote:

> > >         * config/rs6000/e500.h (HARD_REGNO_CALLER_SAVE_MODE): Remove
> > >         macro.
> > >         * config/rs6000/rs6000.h (HARD_REGNO_CALLER_SAVE_MODE): Handle
> > >         TARGET_E500_DOUBLE case here.
> > 
> > This patch is okay.  The repeated testing of E500 seems like it could
> > have been refactored.  The macro is becoming a little overly
> > complicated as a CASE statement.

 I tried avoiding obfuscation, but I agree there's potential for 
improvement here.  I have committed the change now, thanks for your 
review.

> > Are you avoiding the special cases for TFmode and TDmode on e500 for a
> > specific reason or simply matching current behavior?

 I just wanted to fix the rather grave and obscure configuration-induced 
ABI discrepancy bug and otherwise preserved the current behaviour.

> I don't know what's right in the context of the present patch, but the 
> general principle for e500 is that TDmode is much like TImode and DDmode 
> is much like DImode, but TFmode is much like two of DFmode; that was what 
> I concluded when making DFP work for e500 
> <https://gcc.gnu.org/ml/gcc-patches/2008-06/msg00270.html>.

 Joseph, thanks for your input, my understanding of the subtleties of the 
Power ABI is still lacking.

  Maciej

Patch

Index: gcc-fsf-trunk-quilt/gcc/config/rs6000/e500.h
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/rs6000/e500.h	2014-08-21 14:11:19.037911725 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/rs6000/e500.h	2014-08-26 20:37:43.398961962 +0100
@@ -43,12 +43,3 @@ 
 	  error ("E500 and FPRs not supported");			\
       }									\
   } while (0)
-
-/* Override rs6000.h definition.  */
-#undef HARD_REGNO_CALLER_SAVE_MODE
-/* When setting up caller-save slots (MODE == VOIDmode) ensure we
-   allocate space for DFmode.  Save gprs in the correct mode too.  */
-#define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE) \
-  (TARGET_E500_DOUBLE && ((MODE) == VOIDmode || (MODE) == DFmode)	\
-   ? DFmode								\
-   : choose_hard_reg_mode ((REGNO), (NREGS), false))
Index: gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.h
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/rs6000/rs6000.h	2014-08-26 20:30:10.348973028 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.h	2014-08-26 20:37:43.398961962 +0100
@@ -1186,9 +1186,11 @@  enum data_align { align_abi, align_opt, 
    && ((MODE) == VOIDmode || ALTIVEC_OR_VSX_VECTOR_MODE (MODE))		\
    && FP_REGNO_P (REGNO)						\
    ? V2DFmode								\
-   : ((MODE) == TFmode && FP_REGNO_P (REGNO))				\
+   : TARGET_E500_DOUBLE && ((MODE) == VOIDmode || (MODE) == DFmode)	\
    ? DFmode								\
-   : ((MODE) == TDmode && FP_REGNO_P (REGNO))				\
+   : !TARGET_E500_DOUBLE && (MODE) == TFmode && FP_REGNO_P (REGNO)	\
+   ? DFmode								\
+   : !TARGET_E500_DOUBLE && (MODE) == TDmode && FP_REGNO_P (REGNO)	\
    ? DImode								\
    : choose_hard_reg_mode ((REGNO), (NREGS), false))