| Message ID | alpine.DEB.1.10.1409011515330.2958@tp.orcam.me.uk |
|---|---|
| State | Accepted |
| Headers | show |
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
David, This patch: https://gcc.gnu.org/ml/gcc-patches/2014-09/msg00051.html is still waiting, please review. Thanks, Maciej
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
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>.
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
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))