Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Richard Earnshaw <rearnsha at arm dot com>]

On Sat, 2011-01-29 at 00:46 +0800, Jie Zhang wrote:
> On 01/28/2011 10:26 PM, Richard Earnshaw wrote:
> >
> > On Wed, 2010-12-22 at 14:40 +0800, Jie Zhang wrote:
> >> Hi,
> >>
> >> This patch should have no functionality changes. It just moves most of
> >> ARM_LEGITIMIZE_RELOAD_ADDRESS from arm.h to
> >> arm_legitimize_reload_address in arm.c. This is needed by the next
> >> patch. It also eases debugging. Testing is going. OK if the result is good?
> >>
> >> Regards,
> >
> > So I think there's a subtle gotcha in this change that's easy to miss.
> >
> > +&&  REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
> >
> > is a macro that expands into
> >
> > #define REG_MODE_OK_FOR_BASE_P(X, MODE)         \
> >    (TARGET_THUMB1                                \
> >     ? THUMB1_REG_MODE_OK_FOR_BASE_P (X, MODE)    \
> >     : ARM_REG_OK_FOR_BASE_P (X))
> >
> >
> > But THUMB1_REG_MODE_OK_FOR_BASE_P and ARM_REG_OK_FOR_BASE_P both have
> > two possible definitions, that are picked depending upon the file in
> > which the macro is used (a nasty consequence of some attempt to save
> > duplication of code a long long time ago in the early days of GCC).
> >
> > Now IIRC reload.c (which uses legitimize_reload_address) defines
> > REG_OK_STRICT which leads to one definition of these macros, but arm.c
> > does not (and nor should it), which leads to the other definition.
> >
> > Overall, I think that means that your patch has quietly changed the
> > results that this macro can give.
> >
> Good catch! Thanks for review! How about expanding 
> REG_MODE_OK_FOR_BASE_P a bit since we know when REG_MODE_OK_FOR_BASE_P 
> is used in arm_legitimize_reload_address, REG_OK_STRICT is defined and
> TARGET_THUMB1 is false?  In the attached patch, I replace
> 
> +      && REGNO (XEXP (*p, 0)) < FIRST_PSEUDO_REGISTER
> +      && REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
> 
> with
> 
> +      && ARM_REGNO_OK_FOR_BASE_P (REGNO (XEXP (*p, 0)))
> 
> 
> GCC build is OK. But I have not regression tested it yet.
> 
> 

Yes, that sounds sensible.  Consider this approved once testing
completes.

Also, I notice that the same problem has crept into
thumb_legitimize_reload_address.  Perhaps you could correct that too in
a similar manner.  Consider such a patch pre-approved (but please commit
it separately from the ARM one).

R.