Re: [PATCH] reload: Adjust comment in find_reloads about subset, not intersection

[Richard Sandiford <richard.sandiford@arm.com>]

Richard Sandiford <richard.sandiford@arm.com> writes:
> Hans-Peter Nilsson <hp@axis.com> writes:
>>> From: Richard Sandiford <richard.sandiford@arm.com>
>>> Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>> > The mystery isn't so much that there's code mismatching comments or
>>> > intent, but that this code has been there "forever".  There has been a
>>> > function reg_classes_intersect_p, in gcc since r0-54, even *before*
>>> > there was reload.c; r0-361, so why the "we don't have a way of forming
>>> > the intersection" in the actual patch and why wasn't this fixed
>>> > (perhaps not even noticed) when reload was state-of-the-art?
>>> 
>>> But doesn't the comment
>>
>> (the second, patched comment; removed in the patch)
>>
>>> mean that we have/had no way of getting
>>> a *class* that is the intersection of preferred_class[i] and
>>> this_alternative[i], to store as the new
>>> this_alternative[i]?
>>
>> Yes, that's likely what's going on in the (second) comment
>> and code; needing a s/intersection/a class for the
>> intersection/, but the *first* comment is pretty clear that
>> the intent is exactly to "override" this_alternative[i]: "If
>> not (a subclass), but it intersects that class, use the
>> preferred class instead".  But of course, that doesn't
>> actually have to make sense as code!  And indeed it doesn't,
>> as you say.
>>
>>> If the alternatives were register sets rather than classes,
>>> I think the intended effect would be:
>>> 
>>>   this_alternative[i] &= preferred_class[i];
>>> 
>>> (i.e. AND_HARD_REG_SET in old money).
>>> 
>>> It looks like the patch would instead make this_alternative[i] include
>>> registers that the alternative doesn't actually accept, which feels odd.
>>
>> Perhaps I put too much trust in the sanity of old comments.
>>
>> How about I actually commit this one instead?  Better get it
>> right before reload is removed.
>
> :-)  LGTM, but I'd like to hear Jeff's opinion.

So it would be a good idea if I used the right email address.

>
> Thanks,
> Richard
>
>> 8< ------- >8
>> "reload: Adjust comment in find_reloads about subset, not intersection"
>> gcc:
>>
>> 	* reload.cc (find_reloads): Align comment with code where
>> 	considering the intersection of register classes then tweaking the
>> 	regclass for the current alternative or rejecting it.
>> ---
>>  gcc/reload.cc | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/reload.cc b/gcc/reload.cc
>> index 664082a533d9..3ed901e39447 100644
>> --- a/gcc/reload.cc
>> +++ b/gcc/reload.cc
>> @@ -3635,9 +3635,11 @@ find_reloads (rtx_insn *insn, int replace, int ind_levels, int live_known,
>>  		 a hard reg and this alternative accepts some
>>  		 register, see if the class that we want is a subset
>>  		 of the preferred class for this register.  If not,
>> -		 but it intersects that class, use the preferred class
>> -		 instead.  If it does not intersect the preferred
>> -		 class, show that usage of this alternative should be
>> +		 but it intersects that class, we'd like to use the
>> +		 intersection, but the best we can do is to use the
>> +		 preferred class, if it is instead a subset of the
>> +		 class we want in this alternative.  If we can't use
>> +		 it, show that usage of this alternative should be
>>  		 discouraged; it will be discouraged more still if the
>>  		 register is `preferred or nothing'.  We do this
>>  		 because it increases the chance of reusing our spill
>> @@ -3664,9 +3666,10 @@ find_reloads (rtx_insn *insn, int replace, int ind_levels, int live_known,
>>  		  if (! reg_class_subset_p (this_alternative[i],
>>  					    preferred_class[i]))
>>  		    {
>> -		      /* Since we don't have a way of forming the intersection,
>> -			 we just do something special if the preferred class
>> -			 is a subset of the class we have; that's the most
>> +		      /* Since we don't have a way of forming a register
>> +			 class for the intersection, we just do
>> +			 something special if the preferred class is a
>> +			 subset of the class we have; that's the most
>>  			 common case anyway.  */
>>  		      if (reg_class_subset_p (preferred_class[i],
>>  					      this_alternative[i]))