[patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

On 30/04/12 15:39, Richard Sandiford wrote:
> 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
> 


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.

R.