Re: [PATCH][updated] Force SDmode function args into FP registers per the ABI

[Ian Lance Taylor <iant at google dot com>]

Peter Bergner <bergner@vnet.ibm.com> writes:

> To expand a little from our #gcc discussion regarding the movsd problems
> I'm running into with the following patch:
> 
>     http://gcc.gnu.org/ml/gcc-patches/2007-06/msg01289.html
> 
> with the no_new_pseudos/FAIL part removed.  I'm getting an ICE because
> my movsd_hardfloat define_insn doesn't accept movement from MEM<->FPreg
> because of the limitation we talked about.  Ie, we don't have 32-bit
> load/store instructions from the FP registers that will copy the data
> out without destroying it.  Therefore, we need to use 64-bit load/store
> instructions.  The above patch essentially, uses the movsd define_expand
> to break apart the MEM<->FPreg moves to go through a 64-bit stack slot
> and the integer register file.  This part seems to work well.  The patch
> also allocates a 64-bit stack slot for spilling SDmode values, so reload
> should be able to just use the 64-bit load/store instructions directly.
> To perform the 64-bit SDmode load/stores, I added an UNSPEC patterns.
> 
> The problem is, durning reload, instead of generating patterns using
> the UNSPEC for the spills, it uses a normal (set MEM:SD reg:SD) and
> (set REG:SD MEM:SD).  Since the movsd_hardfloat define_insn doesn't
> accept that, we ICE.  What I think I really need is to somehow convince
> reload to generate the UNSPEC pattern instead and things should work.
> 
> After talking with you yesterday, I dug a little deeper to find out
> where the spill load and store insns were coming from.  It seems
> that we call the movsd pattern with the unallocated pseudo and
> not as I thought with the spill slot MEM.  Is it just a matter
> of me looking for reload_in_progress in the movsd pattern and
> changing the insn we create to use the UNSPEC pattern?
> 
> 
> On a related note, is even doing all this expansion during the movsd
> define_expand to blow apart the MEM<->FPreg moves the best way to
> solve this?  Might it be better to just create (set MEM:SD REG:SD)
> and (set REG:SD MEM:SD) copies and then somehow fix them up at
> reload time?  or with define_split's?


What gcc wants to see is a movsd insn or expander which will permit
any combination of moving between register and memory.  Even if you
use an expander, there should still be an insn which does the basic
job; in your case this is movsd_hardfloat.  That insn should use ? and
! in the constraints to guide the preferred register classes for data
moves.

Before register allocation you don't know what class of register the
value will wind up in.  In particular, simple copies of SDmode values
should presumably use the integer registers.  Your code in the patch
cited above is going to pessimize this case.  So I suspect that that
is not the best approach.

Registers which participate in math operations or which are used as
function arguments are going to be forced into the appropriate
register class.  It is then the job of reload to arrange to move those
values into memory or into other register classes.

You are already using SECONDARY_MEMORY_NEEDED and
SECONDARY_MEMORY_NEEDED_RTX.  I assume those are doing the right thing
when copying data across register classes.

The problem, then, is that you are having trouble with the spill code
that reload needs to generate from time to time.  You can't use the
default spill code because you need to use 64-bit load and store
instructions even though your values are only 32 bits wide.

Your patch already arranges for a 64-bit stack slot to use during
spilling.

(Please correct me if I am making a mistake).

So you need to generate the right code for spilling.  That is easy to
do by using TARGET_SECONDARY_RELOAD.  Define insns which do the load
and store that you need.  Define TARGET_SECONDARY_RELOAD to set
sri->icode to those insns when appropriate.  You don't need any
scratch registers, you just need special instructions.

If you want to also simplify movsd as I suggested above, I admit that
the issues become more complicated.  If you want to store an SDmode
value from a floating point register into a 32-bit memory location,
you must presumably stage it through a general register, which
requires two stores and one load.  That is bad but I don't see how you
can avoid it.  And it seems to be what your current code does.

TARGET_SECONDARY_RELOAD should be able to do this too, though not very
conveniently.  One approach would be to add a field to the machine
specific function structure to hold your stack local.  Then call
assign_stack_local to get the 64-bit stack slot you need, and generate
the insns to shuffle the value around.

The problem then becomes that TARGET_SECONDARY_RELOAD will need to
know whether you already have a 64-bit stack slot or not.  Of course
you could use the shuffle for spill code as well, but that would be
pretty ugly.  I think my suggestion here would be to change your
min_stack_slot_size_for_mode target hook.  Instead of returning the
size of a secondary stack slot, have it actually return the stack slot
itself by calling assign_stack_local.  Then you can record those stack
slots in a pointer_set, and have TARGET_SECONDARY_RELOAD check them.
If it finds the entry there, it knows the stack slot is large enough
to avoid the shuffle.

I hope this makes sense and is not just a rathole.

Ian