Richard Earnshaw <rearn...@arm.com> writes:
> On 30/04/12 15:39, Richard Sandiford wrote:
>> Richard Earnshaw <rearn...@arm.com> writes:
>>> On 30/04/12 15:07, Richard Sandiford wrote:
>>>> Richard Earnshaw <rearn...@arm.com> writes:
>>>>> On 26/04/12 14:20, 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
>>>>
>>>
>>> There's no guarantee that all registers in a set are contiguous; ARM for
>>> example doesn't make that guarantee, since SP is not a GP register, but
>>> R12 and R14 are.
>>
>> Sorry, I don't follow. My point was that in_hard_reg_set_p (C, M, R1)
>> tests whether every register required to store a value of mode M starting
>> at R1 fits in C. Which is what we want to know.
>>
>> Whether the intermediate registers (between regno and regno + offset)
>> are even valid for MODE shouldn't matter. I don't think it makes
>> conceptual sense to call:
>>
>> if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
>> mode, i))
>>
>> for regno < i < regno + offset (or regno + offset < i < regno),
>> because we're not trying to construct a value of mode MODE
>> in that register.
>>
>> Richard
>>
>
>
> You're right, of course. I'd missed that in my initial review; and
> hence my follow-up suggestion. It's not particularly interesting
> whether or not HARD_REGISTER_NUM_P (REGNO (operand)) is true, only
> whether REGNO(operand) + offset ... REGNO(operand) + offset +
> NUM_REGS(mode) -1 is.
The problem is that the function currently accepts pseudo registers.
We wouldn't want FIRST_PSEUDO_REGISTER to be accidentally associated
with FIRST_PSUEDO_REGISTER-1, however unlikely that combination of
arguments might be in practice. I think the HARD_REGISTER_NUM_P check
is needed for both regno and regno + offset.
I agree that we should protect against overrun in in_hard_reg_set,
but I think that check logically belongs there. Most targets happen
to be OK because FIRST_PSEUDO_REGISTER isn't divisible by 32,
so the class HARD_REG_SETs always have a terminating zero bit.
There are a couple of exceptions though, such as alpha and lm32.
So one fix would be to require HARD_REG_SET to have an entry
for FIRST_PSEUDO_REGISTER, which wouldn't affect most targets.
Another would be to have an explicit range check in in_hard_reg_set_p
and friends.
Richard
