[patch] More thorough checking in reg_fits_class_p

[Richard Sandiford]

Richard Earnshaw <rearnsha@arm.com> writes:
> On 30/04/12 15:07, Richard Sandiford wrote:
>> Richard Earnshaw <rearnsha@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.macarthur@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