On 2011/4/20 11:12 PM, Richard Earnshaw wrote:
>
> 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 <clt...@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.
The current non-TARGET_LDRD case goes through this path:
...
else
/* For pre-ARMv5TE (without ldrd), we use ldm/stm(db/da/ib)
to access doublewords. The supported load/store offsets are
-8, -4, and 4, which we try to produce here. */
low = ((val & 0xf) ^ 0x8) - 0x8;
which uses ldm/stm. This should be safe.
As for pre-ARMv4 ldrh, this is special cased as:
if (arm_arch4)
low = SIGN_MAG_LOW_ADDR_BITS (val, 8);
else
{
/* The storehi/movhi_bytes fallbacks can use only
[-4094,+4094] of the full ldrb/strb index range. */
low = SIGN_MAG_LOW_ADDR_BITS (val, 12);
if (low == 4095 || low == -4095)
return false;
}
Although to be frank, I haven't really tested a pre-ARMv4 config; not
very easy to do so in an EABI world :)
I'll take the above NEON QREG mode fix as approved.
Chung-Lin
