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

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

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.

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.

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.