Patchwork [RFC] PR 47862: Fix caller-save spill of vectors on PowerPC

login
register
mail settings
Submitter Pat Haugen
Date March 1, 2011, 11:22 p.m.
Message ID <4D6D7FA7.2010306@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/85011/
State New
Headers show

Comments

Pat Haugen - March 1, 2011, 11:22 p.m.
PR47862 documents a problem where we are only saving/restoring half of a vector 
across a call on PowerPC. The lower 32 VSX vector regs (vs0..vs31) overlap the 
legacy floating point registers (F0..F31), but are 16 bytes long instead of 8 
(FP reg overlaps dword0 of corresponding vector reg). The code in 
caller-save.c:init_caller_save() sets up the mode for all caller-save regs 
without regard to what mode is actually in the register at the time of spill. 
Currently DI mode is picked for the floating point regs which leads to only 
saving/restoring half the vector (dword0).

The following patch defines HARD_REGNO_CALLER_SAVE_MODE so that it will return 
V2DF mode for the FP regs for TARGET_VSX. This fixes the issue, but I believe 
has the undesirable effect of causing all FP regs to now be save/restored with 
16-byte vector store/load insns, even if they are only holding scalar floating 
point values. Besides wasting stack space for scalar values, using the vector 
insns can also affect performance due to pipeline/resource constraints.

Are there alternative solutions to this such that we'll use scalar FP store/load 
insns for SF/DF mode spill and vector store/load insns for vector spill? Or is 
the following a correct start on the patch, but more needs to be done to 
setup_save_area() and/or save_call_clobbered_regs() to emit the appropriate 
insns based on mode being spilled (this seems like it could be some non-trivial 
rip up)?

Any comments appreciated.

-Pat
Alan Modra - March 2, 2011, 3:06 a.m.
On Tue, Mar 01, 2011 at 05:22:15PM -0600, Pat Haugen wrote:
> Are there alternative solutions to this such that we'll use scalar
> FP store/load insns for SF/DF mode spill and vector store/load insns
> for vector spill?

I made a similar fix a while ago in rs6000/e500.h (which your patch
will clash with, BTW, needs an undef somewhere).  Going from memory, I
believe you can use something like

#define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE)	\
  (TARGET_VSX						\
   && ((MODE) == VOIDmode || (MODE) == V2DFmode)	\
   && FP_REGNO_P (REGNO)				\
   ? V2DFmode						\
   : choose_hard_reg_mode ((REGNO), (NREGS), false))

This will allocate enough space for the vector regs, but only
save/restore using FP insns if the reg is used in a FP mode.
Pat Haugen - March 2, 2011, 5:53 p.m.
On 03/01/2011 09:06 PM, Alan Modra wrote:
> On Tue, Mar 01, 2011 at 05:22:15PM -0600, Pat Haugen wrote:
>> >  Are there alternative solutions to this such that we'll use scalar
>> >  FP store/load insns for SF/DF mode spill and vector store/load insns
>> >  for vector spill?
> I made a similar fix a while ago in rs6000/e500.h (which your patch
> will clash with, BTW, needs an undef somewhere).  Going from memory, I
> believe you can use something like
>
> #define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE)	\
>    (TARGET_VSX						\
>     &&  ((MODE) == VOIDmode || (MODE) == V2DFmode)	\
>     &&  FP_REGNO_P (REGNO)				\
>     ? V2DFmode						\
>     : choose_hard_reg_mode ((REGNO), (NREGS), false))
>
> This will allocate enough space for the vector regs, but only
> save/restore using FP insns if the reg is used in a FP mode.
>

I don't see how this can result in shortening of the save/restore length. 
caller-save.c:save_call_clobbered_regs() initally sets save_mode[] to the prior 
computed widest mode (V2DFmode in this case), and the following code will only 
attempt to increase it.

                   mode = HARD_REGNO_CALLER_SAVE_MODE
                     (r, nregs, PSEUDO_REGNO_MODE (regno));
                   if (GET_MODE_BITSIZE (mode)
                       > GET_MODE_BITSIZE (save_mode[r]))
                     save_mode[r] = mode;

Modifying the above code to always update save_mode[r] to the mode of the pseudo 
would surely do the trick.
Pat Haugen - March 2, 2011, 10:32 p.m.
On 03/02/2011 11:53 AM, Pat Haugen wrote:
> I don't see how this can result in shortening of the save/restore length.
> caller-save.c:save_call_clobbered_regs() initally sets save_mode[] to the prior
> computed widest mode (V2DFmode in this case), and the following code will only
> attempt to increase it.

Forget this comment, I just created a testcase for a FP reg spill and it does in 
fact use the scalar FP store/load insns.

Patch

Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h  (revision 170438)
+++ gcc/config/rs6000/rs6000.h  (working copy)
@@ -1005,6 +1005,12 @@  extern unsigned rs6000_pointer_size;

  #define HARD_REGNO_NREGS(REGNO, MODE) rs6000_hard_regno_nregs[(MODE)][(REGNO)]

+/* Ensure vector modes are handled correctly in FP regs for VSX */
+#define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE)        \
+  (TARGET_VSX && FP_REGNO_P (REGNO)                    \
+   ? V2DFmode                                          \
+   : choose_hard_reg_mode ((REGNO), (NREGS), false))
+
  #define HARD_REGNO_CALL_PART_CLOBBERED(REGNO, MODE)                    \
    (((TARGET_32BIT && TARGET_POWERPC64                                  \
       && (GET_MODE_SIZE (MODE) > 4)                                     \