Re: [patch, ARM] PR48250, rehaul arm_legitimize_reload_address()

[Richard Earnshaw <rearnsha@arm.com>]

On Wed, 2011-04-20 at 23:06 +0800, Chung-Lin Tang wrote:
> On 2011/4/20 09:24 PM, Richard Sandiford wrote:
> > Hi Chung-Lin,
> > 
> > I'm seeing an ICE with this patch, specifically;
> > 
> > Chung-Lin Tang <cltang@codesourcery.com> writes:
> >> +      if (coproc_p)
> >> +	low = SIGN_MAG_LOW_ADDR_BITS (val, 10);
> > 
> > We generate:
> > 
> > Reload 1: reload_out (V4SI) = (mem/c:V4SI (plus:SI (plus:SI (reg/f:SI 11 fp)
> >                                                             (const_int -6144 [0xffffffffffffe800]))
> >                                                         (const_int 1020 [0x3fc])) [43 %sfp+-5024 S16 A64])
> > 
> > but 1020 isn't a legitimate offset for V4SI:
> > 
> >   /* For quad modes, we restrict the constant offset to be slightly less
> >      than what the instruction format permits.  We do this because for
> >      quad mode moves, we will actually decompose them into two separate
> >      double-mode reads or writes.  INDEX must therefore be a valid
> >      (double-mode) offset and so should INDEX+8.  */
> >   if (TARGET_NEON && VALID_NEON_QREG_MODE (mode))
> >     return (code == CONST_INT
> > 	    && INTVAL (index) < 1016
> > 	    && INTVAL (index) > -1024
> > 	    && (INTVAL (index) & 3) == 0);
> > 
> > A simple "fix" would be to use 9 instead of 10, but something a little
> > more subtle might be preferred :-)
> > 
> > Richard
> 
> Oh dear, for some reason I mistakenly thought that NEON had a quad-word
> load/store, sorry :P
> 
> Reducing from 10 to 9 may be a possible solution, if restricted to the
> necessary cases. For example:
> 
> -low = SIGN_MAG_LOW_ADDR_BITS (val, 10);
> +{
> +  low = SIGN_MAG_LOW_ADDR_BITS (val, 10);
> +
> +  /* NEON quad-word load/stores are made of two double-word accesses,
> +     so the valid index range is reduced by 8. Treat as 9-bit range if
> +     we go over it.  */
> +  if (TARGET_NEON && VALID_NEON_QREG_MODE (mode) && low >= 1016)
> +    low = SIGN_MAG_LOW_ADDR_BITS (val, 9);
> +}
> 
> To Richard Earnshaw, how do you think of a fix like this? Or should we
> just simply return false under this out-of-range case (it should be rare
> I hope).
> 

I don't think it matters a great deal.  The above is fine.

Note, that some targets don't have LDRD either.  Do we do the right
thing if we're going to fall back to two LDR instructions?

R.