[patch, ARM] Fix PR48250, adjust DImode reload address legitimizing

[Richard Earnshaw]

On Tue, 2011-03-29 at 18:25 +0800, Chung-Lin Tang wrote:
> On 2011/3/24 06:51 PM, Richard Earnshaw wrote:
> > 
> > On Thu, 2011-03-24 at 12:56 +0900, Chung-Lin Tang wrote:
> >> Hi,
> >> PR48250 happens under TARGET_NEON, where DImode is included within the
> >> valid NEON modes. This turns the range of legitimate constant indexes to
> >> step-4 (coproc load/store), thus arm_legitimize_reload_address() when
> >> trying to decompose the [reg+index] reload address into
> >> [(reg+index_high)+index_low], can cause an ICE later when 'index_low'
> >> part is not aligned to 4.
> >>
> >> I'm not sure why the current DImode index is computed as:
> >> low = ((val & 0xf) ^ 0x8) - 0x8;  the sign-extending into negative
> >> values, then subtracting back, actually creates further off indexes.
> >> e.g. in the supplied testcase, [sp+13] was turned into [(sp+16)-3].
> >>
> > 
> > Hysterical Raisins... the code there was clearly written for the days
> > before we had LDRD in the architecture.  At that time the most efficient
> > way to load a 64-bit object was to use the LDM{ia,ib,da,db}
> > instructions.  The computation here was (I think), intended to try and
> > make the most efficient use of an add/sub instruction followed by
> > LDM/STM offsetting.  At that time the architecture had no unaligned
> > access either, so dealing with 64-bit that were less than 32-bit aligned
> > (in those days 32-bit was the maximum alignment) probably wasn't
> > considered, or couldn't even get through to reload.
> >
> 
> I see it now. The code in output_move_double() returning assembly for
> ldm/stm(db/da/ib) for offsets -8/-4/+4 seems to confirm this.
> 
> I have changed the patch to let the new code handle the TARGET_LDRD case
> only.  The pre-LDRD case is still handled by the original code, with an
> additional & ~0x3 for aligning the offset to 4.
> 
> I've also added a comment for the pre-TARGET_LDRD case. Please see if
> the description is accurate enough.
> 
> >> My patch changes the index decomposing to a more straightforward way; it
> >> also sort of outlines the way the other reload address indexes are
> >> broken by using and-masks, is not the most effective.  The address is
> >> computed by addition, subtracting away the parts to obtain low+high
> >> should be the optimal way of giving the largest computable index range.
> >>
> >> I have included a few Thumb-2 bits in the patch; I know currently
> >> arm_legitimize_reload_address() is only used under TARGET_ARM, but I
> >> guess it might eventually be turned into TARGET_32BIT.
> >>
> > 
> > I think this needs to be looked at carefully on ARMv4/ARMv4T to check
> > that it doesn't cause regressions there when we don't have LDRD in the
> > instruction set.
> 
> I'll be testing the modified patch under an ARMv4/ARMv4T configuration.
> Okay for trunk if no regressions?
> 
> Thanks,
> Chung-Lin
> 
> 	PR target/48250
> 	* config/arm/arm.c (arm_legitimize_reload_address): Adjust
> 	DImode constant index decomposing under TARGET_LDRD. Clear
> 	lower two bits for NEON, Thumb-2, and !TARGET_LDRD. Add
> 	comment for !TARGET_LDRD case.

This looks technically correct, but I can't help feeling that trying to
deal with the bottom two bits being non-zero is not really worthwhile.
This hook is an optimization that's intended to generate better code
than the default mechanisms that reload provides.  It is allowed to
fail, but it must say so if it does (by returning false).

What this hook is trying to do for DImode is to take an address of the
form (reg + TOO_BIG_CONST) and turn it into two instructions that are
legitimate:

	tmp = (REG + LEGAL_BIG_CONST)
	some_use_of (mem (tmp + SMALL_LEGAL_CONST))

The idea behind the optimization is that LEGAL_BIG_CONST will need fewer
instructions to generate than TOO_BIG_CONST.  It's unlikely (probably
impossible in ARM state) that pushing the bottom two bits of the address
into LEGAL_BIG_CONST part of the offset is going to lead to a better
code sequence.  

So I think it would be more sensible to just return false if we have a
DImode address with the bottom 2 bits non-zero and then let the normal
reload recovery mechanism take over.