Re: [RTL, ColdFire 24/63] Add support for a MODE_INDEX_REG_CLASS macro

[Richard Sandiford <richard at codesourcery dot com>]

Jeff, just to confirm...

Richard Sandiford <richard@codesourcery.com> writes:
> Jeffrey Law <law@redhat.com> writes:
>> On Wed, 2007-01-10 at 21:27 +0000, Richard Sandiford wrote:
>>> Right, that describes the ISA restrictions.  But this patch isn't
>>> really about the ISA restrictions per se.  The m68k port makes a pragmatic
>>> decision to forbid indexed addressing modes for _all_ SFmode and DFmode
>>> MEMs if TARGET_COLDFIRE_FPU, on the basis that the vast majority of
>>> memory accesses will be in FPU instructions.  We then expose the
>>> necessary addressing code to the pre-reload optimisers and register
>>> allocators, rather than leaving reload to find scratch address registers
>>> from somewhere.  Although I didn't make that decision (it's what current
>>> mainline does), I can well imagine that it leads to better code.
>>> 
>>> In other words, this is entirely an internal gcc thing.  The current
>>> definition of INDEX_REG_CLASS tricks reload into thinking that indexed
>>> addressing modes are acceptable SFmode and DFmode memory_operands,
>>> which they aren't.  It will then use such memories in copy-in and
>>> copy-out reload instructions.
>> The fundamental problem is that the m68k port is lying and you're
>> hacking up reload to deal with the m68k version of the lie.
>
> I disagree that this patch is a hack.  I think the only real liar here
> is INDEX_REG_CLASS, which says that something is a valid index register
> even when GO_IF_LEGITIMATE_ADDRESS forbids it.  I think the situation
> after the patch is self-consistent and the correct approach to take
> (in terms of what addresses are and aren't acceptable).
>
>> FWIW, the PA port lies too, but all the hair of the lie is buried in
>> the backend.  You might review how the PA handles this situation since
>> they are quite similar (basically reversed -- the PA has full indexed
>> addressing modes for loads/stores using FP registers, but not for
>> integer registers.
>
> To recap, the original problem was that reload creates copy-in reloads
> from (or copy-out reloads to) invalid indexed addresses.  This can
> happen when a spill slot MEM has an out-of-range offset, for example.
>
> If I've understood correctly, the PA port only avoids this problem
> for SImode MEMs because it defines LEGITIMIZE_RELOAD_ADDRESS.
> L_R_A deals with out-of-range offsets by adding the high part of the
> offset to the base and then adding the left-over offset.  I'm sure that
> this definition was added purely for the sake of optimisation, but it has
> the side-effect of stopping reload from loading out-of-range offsets into
> index registers.  Would everything still work without L_R_A?  If not,
> that seems a little suspect.  I'd always been told that L_R_A should
> only be used for optimisation, and that it was wrong to rely on it for
> correctness.
>
> The wider issue seems to be: should we either
>
>   (a) allow indexed addresses for all modes and use constraints and
>       secondary reloads to handle the cases where they aren't allowed or
>
>   (b) prevent the use of index registers for modes if the vast majority
>       of MEMs in those modes are likely to not be index registers.
>
> (a) is what PA does and (b) is what m68k does.  I think (b) is the
> right choice for the reason outlined in the quote above: if we allow
> indexed MEMs before reload, it's up to reload to convert indexed MEMs
> into legitimate addresses.  To do that it needs some scratch register,
> which it will pick in an ad-hoc way.  It's better to expose the scratch
> register before reload if possible, and that's what (b) does.
> Note that the effect is likely to be more obvious on m68k than PA
> because the m68k port sometimes has only 5 allocatable address registers
> (after you've taken away the stack pointer, frame pointer and
> PIC register).
>
> Note that the m68k port already deals with SImode indexed MEMs in the
> same way as the PA.  It accepts indexed addresses and use constraints
> to restrict the choice.

...do you still object to this MODE_INDEX_REG_CLASS patch, or is it
OK to go ahead and commit it?  (Ian has already approved it -- thanks --
but I wanted to check whether it was still contentious.)

Richard