[patch] More thorough checking in reg_fits_class_p

[patch] More thorough checking in reg_fits_class_p

[Jim MacArthur]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

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.


[-- Attachment #2: reg-fits-class-9 --]
[-- Type: text/plain, Size: 1128 bytes --]

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;
+	}
+      return true;
+    }
+  return false;
 }
 \f
 /* Split single instruction.  Helper function for split_all_insns and

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

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;
> +	}
> +      return true;
> +    }
> +  return false;
>  }
>  \f
>  /* Split single instruction.  Helper function for split_all_insns and

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Sandiford]

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

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

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.

R.

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

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
> 

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

R.

Re: [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

Re: [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.

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Sandiford]

Richard Earnshaw <rearnsha@arm.com> writes:
> 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.

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

Re: [patch] More thorough checking in reg_fits_class_p

[Georg-Johann Lay]

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.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
> 
> 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

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

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.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
>>
>> 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.

Re: [patch] More thorough checking in reg_fits_class_p

[Jim MacArthur]

[-- Attachment #1: Type: text/plain, Size: 5658 bytes --]

On 30/04/12 16:19, Richard Sandiford wrote:
> Richard Earnshaw<rearnsha@arm.com>  writes:
>> 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.
> 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
>
>
Thanks for all your comments. I've made a new version which checks all 
three cases (regno, regno+offset, end_hard_regno (regno+offset, M) ) 
with HARD_REGISTER_NUM_P but returns to just checking regno + offset 
with in_hard_reg_set_p. How does this look?

New Changelog text:

2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
* recog.c (reg_fits_class_p): Check both regno and regno + offset are
hard registers.



[-- Attachment #2: reg-fits-class-10 --]
[-- Type: text/plain, Size: 843 bytes --]

diff --git a/gcc/recog.c b/gcc/recog.c
index 8fb96a0..a416e06 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2759,14 +2759,17 @@ 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;
 
+  /* Regno must not be a pseudo register.  Offset may be negative.  */
   return (HARD_REGISTER_NUM_P (regno)
-	  && in_hard_reg_set_p (reg_class_contents[(int) cl],
-				mode, regno + offset));
+	  && HARD_REGISTER_NUM_P (regno + offset)
+	  && HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
+	  && in_hard_reg_set_p (reg_class_contents[(int) cl], mode, 
+				regno + offset));
 }
 \f
 /* Split single instruction.  Helper function for split_all_insns and

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Sandiford]

Jim MacArthur <jim.macarthur@arm.com> writes:
> New Changelog text:
>
> 2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
> * recog.c (reg_fits_class_p): Check both regno and regno + offset are
> hard registers.

Thanks.  I still think the final:

> +	  && HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))

check belongs in in_hard_reg_set_p, since most callers don't (and IMO
shouldn't need to) check this.  The idea behind adding these functions
was to commonise various bits of code that were doing the same checks
in slightly different ways.  Requiring each caller to check the end
register would go against that to some extent.

E.g. the code could be:

  end_regno = end_hard_regno (mode, regno);
  if (!HARD_REGISTER_NUM_P (end_regno))
    return false;

  while (++regno < end_regno)
    if (!TEST_HARD_REG_BIT (regs, regno))
      return false;

Or alternatively we could change hard-reg-set.h so that HARD_REG_SET
is big enough to store a bit for FIRST_PSEUDO_REGISTER.  I don't mind
which, but others might.

Looks good otherwise.  And for the record, I can't approve this anyway. :-)

Richard

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

On 02/05/12 14:00, Richard Sandiford wrote:
> Jim MacArthur <jim.macarthur@arm.com> writes:
>> New Changelog text:
>>
>> 2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
>> * recog.c (reg_fits_class_p): Check both regno and regno + offset are
>> hard registers.
> 
> Thanks.  I still think the final:
> 
>> +	  && HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
> 
> check belongs in in_hard_reg_set_p, since most callers don't (and IMO
> shouldn't need to) check this.  The idea behind adding these functions
> was to commonise various bits of code that were doing the same checks
> in slightly different ways.  Requiring each caller to check the end
> register would go against that to some extent.
> 

If you're going to do that (which is fine, BTW), I think
in_hard_reg_set_p should gcc_assert() that regno is a valid hard reg.

> E.g. the code could be:
> 
>   end_regno = end_hard_regno (mode, regno);
>   if (!HARD_REGISTER_NUM_P (end_regno))
>     return false;
> 
>   while (++regno < end_regno)
>     if (!TEST_HARD_REG_BIT (regs, regno))
>       return false;
> 
> Or alternatively we could change hard-reg-set.h so that HARD_REG_SET
> is big enough to store a bit for FIRST_PSEUDO_REGISTER.  I don't mind
> which, but others might.

I'd prefer the test to be explicit.  The buffer overrun check might be a
tad more efficient in some cases, but someday someone will manage to
find a way of directly going 2 regs beyond the end...

> 
> Looks good otherwise.  And for the record, I can't approve this anyway. :-)
> 
> Richard
> 

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Sandiford]

Richard Earnshaw <rearnsha@arm.com> writes:
> On 02/05/12 14:00, Richard Sandiford wrote:
>> Jim MacArthur <jim.macarthur@arm.com> writes:
>>> New Changelog text:
>>>
>>> 2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
>>> * recog.c (reg_fits_class_p): Check both regno and regno + offset are
>>> hard registers.
>> 
>> Thanks.  I still think the final:
>> 
>>> +	  && HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
>> 
>> check belongs in in_hard_reg_set_p, since most callers don't (and IMO
>> shouldn't need to) check this.  The idea behind adding these functions
>> was to commonise various bits of code that were doing the same checks
>> in slightly different ways.  Requiring each caller to check the end
>> register would go against that to some extent.
>> 
>
> If you're going to do that (which is fine, BTW), I think
> in_hard_reg_set_p should gcc_assert() that regno is a valid hard reg.

Sounds good.

Richard

Re: [patch] More thorough checking in reg_fits_class_p

[Jim MacArthur]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On 02/05/12 14:55, Richard Sandiford wrote:
> Richard Earnshaw<rearnsha@arm.com>  writes:
>> On 02/05/12 14:00, Richard Sandiford wrote:
>>> Jim MacArthur<jim.macarthur@arm.com>  writes:
>>>> New Changelog text:
>>>>
>>>> 2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
>>>> * recog.c (reg_fits_class_p): Check both regno and regno + offset are
>>>> hard registers.
>>> Thanks.  I still think the final:
>>>
>>>> +	&&  HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
>>> check belongs in in_hard_reg_set_p, since most callers don't (and IMO
>>> shouldn't need to) check this.  The idea behind adding these functions
>>> was to commonise various bits of code that were doing the same checks
>>> in slightly different ways.  Requiring each caller to check the end
>>> register would go against that to some extent.
>>>
>> If you're going to do that (which is fine, BTW), I think
>> in_hard_reg_set_p should gcc_assert() that regno is a valid hard reg.
> Sounds good.
>
> Richard
>

Sorry for the delay in responding to this, I had a few problems with 
end_hard_regno. Here's a new version of the patch, which adds to 
in_hard_reg_set_p the assert and a check for the hardness of end_regno.  
end_hard_regno is the exclusive upper bound of the range, so not 
actually a meaningful reg no. HARD_REGNO_NREGS is required to return a 
positive value, so (end_regno - 1) is always safe, as I understand it.

I've tested this with an x86 bootstrap which shows no errors, and with 
our own AArch64 back end.

Jim

New ChangeLog text:

2012-05-17  Jim MacArthur<jim.macarthur@arm.com>
       * recog.c (reg_fits_class_p): Check both regno and regno + offset are  
hard registers.
       * regs.h (in_hard_reg_set_p): Assert that regno is a hard register and
check end_regno - 1 is a hard register.


[-- Attachment #2: reg-fits-class-16 --]
[-- Type: text/plain, Size: 1541 bytes --]

diff --git a/gcc/recog.c b/gcc/recog.c
index c5725d2..d664594 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2792,14 +2792,16 @@ 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;
 
+  /* Regno must not be a pseudo register.  Offset may be negative.  */
   return (HARD_REGISTER_NUM_P (regno)
-	  && in_hard_reg_set_p (reg_class_contents[(int) cl],
-				mode, regno + offset));
+	  && HARD_REGISTER_NUM_P (regno + offset)
+	  && in_hard_reg_set_p (reg_class_contents[(int) cl], mode, 
+				regno + offset));
 }
 \f
 /* Split single instruction.  Helper function for split_all_insns and
diff --git a/gcc/regs.h b/gcc/regs.h
index 328b839..d18bf0a 100644
--- a/gcc/regs.h
+++ b/gcc/regs.h
@@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #include "machmode.h"
 #include "hard-reg-set.h"
+#include "rtl.h"
 
 #define REG_BYTES(R) mode_size[(int) GET_MODE (R)]
 
@@ -367,10 +368,16 @@ in_hard_reg_set_p (const HARD_REG_SET regs, enum machine_mode mode,
 {
   unsigned int end_regno;
 
+  gcc_assert (HARD_REGISTER_NUM_P (regno));
+  
   if (!TEST_HARD_REG_BIT (regs, regno))
     return false;
 
   end_regno = end_hard_regno (mode, regno);
+
+  if (!HARD_REGISTER_NUM_P (end_regno - 1))
+    return false;
+
   while (++regno < end_regno)
     if (!TEST_HARD_REG_BIT (regs, regno))
       return false;

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Sandiford]

Jim MacArthur <jim.macarthur@arm.com> writes:
> On 02/05/12 14:55, Richard Sandiford wrote:
>> Richard Earnshaw<rearnsha@arm.com>  writes:
>>> On 02/05/12 14:00, Richard Sandiford wrote:
>>>> Jim MacArthur<jim.macarthur@arm.com>  writes:
>>>>> New Changelog text:
>>>>>
>>>>> 2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
>>>>> * recog.c (reg_fits_class_p): Check both regno and regno + offset are
>>>>> hard registers.
>>>> Thanks.  I still think the final:
>>>>
>>>>> +	&&  HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
>>>> check belongs in in_hard_reg_set_p, since most callers don't (and IMO
>>>> shouldn't need to) check this.  The idea behind adding these functions
>>>> was to commonise various bits of code that were doing the same checks
>>>> in slightly different ways.  Requiring each caller to check the end
>>>> register would go against that to some extent.
>>>>
>>> If you're going to do that (which is fine, BTW), I think
>>> in_hard_reg_set_p should gcc_assert() that regno is a valid hard reg.
>> Sounds good.
>>
>> Richard
>>
>
> Sorry for the delay in responding to this, I had a few problems with 
> end_hard_regno. Here's a new version of the patch, which adds to 
> in_hard_reg_set_p the assert and a check for the hardness of end_regno.  
> end_hard_regno is the exclusive upper bound of the range, so not 
> actually a meaningful reg no. HARD_REGNO_NREGS is required to return a 
> positive value, so (end_regno - 1) is always safe, as I understand it.
>
> I've tested this with an x86 bootstrap which shows no errors, and with 
> our own AArch64 back end.

Since I kicked up a fuss: looks good to me, thanks.  Richard would have
to be the one to approve it though.

Richard

Re: [patch] More thorough checking in reg_fits_class_p

[Richard Earnshaw]

On 17/05/12 14:23, Jim MacArthur wrote:
> On 02/05/12 14:55, Richard Sandiford wrote:
>> Richard Earnshaw<rearnsha@arm.com>  writes:
>>> On 02/05/12 14:00, Richard Sandiford wrote:
>>>> Jim MacArthur<jim.macarthur@arm.com>  writes:
>>>>> New Changelog text:
>>>>>
>>>>> 2012-05-02 Jim MacArthur<jim.macarthur@arm.com>
>>>>> * recog.c (reg_fits_class_p): Check both regno and regno + offset are
>>>>> hard registers.
>>>> Thanks.  I still think the final:
>>>>
>>>>> +	&&  HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
>>>> check belongs in in_hard_reg_set_p, since most callers don't (and IMO
>>>> shouldn't need to) check this.  The idea behind adding these functions
>>>> was to commonise various bits of code that were doing the same checks
>>>> in slightly different ways.  Requiring each caller to check the end
>>>> register would go against that to some extent.
>>>>
>>> If you're going to do that (which is fine, BTW), I think
>>> in_hard_reg_set_p should gcc_assert() that regno is a valid hard reg.
>> Sounds good.
>>
>> Richard
>>
> 
> Sorry for the delay in responding to this, I had a few problems with 
> end_hard_regno. Here's a new version of the patch, which adds to 
> in_hard_reg_set_p the assert and a check for the hardness of end_regno.  
> end_hard_regno is the exclusive upper bound of the range, so not 
> actually a meaningful reg no. HARD_REGNO_NREGS is required to return a 
> positive value, so (end_regno - 1) is always safe, as I understand it.
> 
> I've tested this with an x86 bootstrap which shows no errors, and with 
> our own AArch64 back end.
> 
> Jim
> 
> New ChangeLog text:
> 
> 2012-05-17  Jim MacArthur<jim.macarthur@arm.com>
>        * recog.c (reg_fits_class_p): Check both regno and regno + offset are  
> hard registers.
>        * regs.h (in_hard_reg_set_p): Assert that regno is a hard register and
> check end_regno - 1 is a hard register.
> 

OK.

R.

Re: [patch] More thorough checking in reg_fits_class_p

[Marcus Shawcroft]

On 21/05/12 15:47, Richard Earnshaw wrote:
> On 17/05/12 14:23, Jim MacArthur wrote:
>>
>> Sorry for the delay in responding to this, I had a few problems with
>> end_hard_regno. Here's a new version of the patch, which adds to
>> in_hard_reg_set_p the assert and a check for the hardness of end_regno.
>> end_hard_regno is the exclusive upper bound of the range, so not
>> actually a meaningful reg no. HARD_REGNO_NREGS is required to return a
>> positive value, so (end_regno - 1) is always safe, as I understand it.
>>
>> I've tested this with an x86 bootstrap which shows no errors, and with
>> our own AArch64 back end.
>>
>> Jim
>>
>> New ChangeLog text:
>>
>> 2012-05-17  Jim MacArthur<jim.macarthur@arm.com>
>>         * recog.c (reg_fits_class_p): Check both regno and regno + offset are
>> hard registers.
>>         * regs.h (in_hard_reg_set_p): Assert that regno is a hard register and
>> check end_regno - 1 is a hard register.
>>
>
> OK.
>
> R.
>
>

Jim, I've committed your patch.

/Marcus