On 30/04/12 16:36, Georg-Johann Lay wrote:
> Richard Earnshaw schrieb:
>> On 30/04/12 15:07, Richard Sandiford wrote:
>>
>>> Richard Earnshaw writes:
>>>
>>>> Jim MacArthur wrote:
>>>>
>>>>> The current code in reg_fits_class_p appears to be incorrect; since
>>>>> offset may be negative, it's necessary to check both ends of the range
>>>>> otherwise an array overrun or underrun may occur when calling
>>>>> in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each
>>>>> register in the range of regno .. regno+offset.
>>>>>
>>>>> A negative offset can occur on a big-endian target. For example, on
>>>>> AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1.
>>>>>
>>>>> We discovered this problem while developing unrelated code for
>>>>> big-endian support in the AArch64 back end.
>>>>>
>>>>> I've tested this with an x86 bootstrap which shows no errors, and with
>>>>> our own AArch64 back end.
>>>>>
>>>>> Ok for trunk?
>>>>>
>>>>> gcc/Changelog entry:
>>>>>
>>>>> 2012-04-26 Jim MacArthur<jim.macart...@arm.com>
>>>>> * recog.c (reg_fits_class_p): Check every register between regno and
>>>>> regno+offset is in the hard register set.
>>>>
>>>> OK.
>>>>
>>>> R.
>>>>
>>>>> reg-fits-class-9
>>>>>
>>>>> diff --git a/gcc/recog.c b/gcc/recog.c
>>>>> index 8fb96a0..825bfb1 100644
>>>>> --- a/gcc/recog.c
>>>>> +++ b/gcc/recog.c
>>>>> @@ -2759,14 +2759,28 @@ bool
>>>>> reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset,
>>>>> enum machine_mode mode)
>>>>> {
>>>>> - int regno = REGNO (operand);
>>>>> + unsigned int regno = REGNO (operand);
>>>>>
>>>>> if (cl == NO_REGS)
>>>>> return false;
>>>>>
>>>>> - return (HARD_REGISTER_NUM_P (regno)
>>>>> - && in_hard_reg_set_p (reg_class_contents[(int) cl],
>>>>> - mode, regno + offset));
>>>>> + /* We should not assume all registers in the range regno to regno +
>>>>> offset are
>>>>> + valid just because each end of the range is. */
>>>>> + if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno +
>>>>> offset))
>>>>> + {
>>>>> + unsigned int i;
>>>>> +
>>>>> + unsigned int start = MIN (regno, regno + offset);
>>>>> + unsigned int end = MAX (regno, regno + offset);
>>>>> + for (i = start; i <= end; i++)
>>>>> + {
>>>>> + if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
>>>>> + mode, i))
>>>>> + return false;
>>>>> + }
>>>
>>> This doesn't look right to me. We should still only need to check
>>> in_hard_reg_set_p for one register number. I'd have expected
>>> something like:
>>>
>>> return (HARD_REGISTER_NUM_P (regno)
>>> && HARD_REGISTER_NUM_P (regno + offset)
>>> && in_hard_reg_set_p (reg_class_contents[(int) cl],
>>> mode, regno + offset));
>>>
>>> instead.
>>>
>>> Richard
>>
>> Hmm, looking at the comment that precedes the function makes me think
>> the actual implementation should be:
>>
>> {
>> int regno = REGNO (operand) + offset;
>> ...
>>
>> return (HARD_REGISTER_NUM_P (regno)
>> && HARD_REGISTER_NUM_P (end_hard_regno (regno, mode))
>> && in_hard_reg_set_p (...., regno);
>>
>> }
>>
>> That is, the original regno isn't really interesting and what really
>> counts is the range regno + offset ... regno + offset +
>> NUM_HARD_REGS(mode) - 1
>
> Shouldn't this be HARD_REGNO_NREGS?
>
> Johann
>
Look at the definition of end_hard_regno...
R.
