[PATCH] reload: Adjust find_reloads to comment; test intersection, not subset

[PATCH] reload: Adjust find_reloads to comment; test intersection, not subset

[Hans-Peter Nilsson]

I'm not seriously submitting this patch for approval.  I just thought
it'd be interesting to some people, at least those maintaining ports
still using reload; I know it's reload and major ports don't really
care about that anymore.  TL;DR: scroll down for the mystery part.

The *benevolent* effects of this bug had me confused for a while: I
thought I'd see neutral or good effects of nominally *improving* the
register-class topology of the CRIS port such that there's "a class
for the union of two classes whenever some instruction allows both
classes".  Essentially, IRA picked (still does, not committed yet) two
classes GENNONACR_REGS and SPECIAL_REGS and SPEC_GENNONACR_REGS, their
union, but the move-pattern alternatives corresponding to
GENNONACR_REGS are actually "r" (GENERAL_REGS), which also contains
the ACR register.  I want to get rid of all things related to ACR and
simplify the port, as ACR is an artefact of a no-longer-supported CRIS
variant (see commit r11-220 which also mentions this remaining
artefact).

But, when improving the topology, instead of still making good use of
registers from SPECIAL_REGS where the reload pass matters, registers
from GENERAL_REGS are always preferred.  Though, for coremark, this
only happens in two printf-formatting functions of newlib; _vfprintf_r
and _vfiprintf_r (same code actually, and it's likely a bug that both
instances get included).

Anyway, skipping to the point: the effect of the bug is that
alternatives corresponding to classes that are *not subclasses* of the
preferred class (the union), are demoted by 4 (or actually: 2 + 2 *
pref_or_nothing[i]), instead of those register alternatives where the
register-class *intersection is empty*, as is the intent according to
my reading of the comment.  For CRIS, this means that GENERAL_REGS get
demoted (as ACR is present there but not in SPEC_GENNONACR_REGS) and
then SPECIAL_REGS are preferred, which is *sometimes* better as those
registers are "free".  They're not used in regular code and outside
their special purposes, basically just usable for moving around data;
perfect for plain single-use spill-reloads.  Net result: for CRIS the
bug causes a slightly smaller coremark binary but the same execution
time.

With the CRIS register-class topology fixed and the bug no longer
having effect, I have to adjust the port such that SPECIAL_REGS are
preferred when that helps (some heuristics required), as the (current)
order of alternatives for the *movsi_internal pattern causes
preference of GENERAL_REGS over SPECIAL_REGS when the cost is the
same.  IOW, a flaw in the port masked by a bug in reload.

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?

(I know the file has been renamed but again this isn't an patch
intended to be applied.  The context is adjusted to include the
comment.  The ChangeLog entry is the effect of a habitual twitch. 8-]
)

gcc:
	* reload.c (find_reloads): Call reg_classes_intersect_p, not
	reg_class_subset_p.
---
 gcc/reload.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gcc/reload.c b/gcc/reload.c
index 4c55ca58a5f7..36e887c6b57e 100644
--- a/gcc/reload.c
+++ b/gcc/reload.c
@@ -3616,43 +3616,39 @@ find_reloads (rtx_insn *insn, int replace, int ind_levels, int live_known,
 	      /* If this operand is a pseudo register that didn't get
 		 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
 		 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
 		 register in a later insn and avoiding a pair of
 		 memory stores and loads.
 
 		 Don't bother with this if this alternative will
 		 accept this operand.
 
 		 Don't do this for a multiword operand, since it is
 		 only a small win and has the risk of requiring more
 		 spill registers, which could cause a large loss.
 
 		 Don't do this if the preferred class has only one
 		 register because we might otherwise exhaust the
 		 class.  */
 
 	      if (! win && ! did_match
 		  && this_alternative[i] != NO_REGS
 		  && known_le (GET_MODE_SIZE (operand_mode[i]), UNITS_PER_WORD)
 		  && reg_class_size [(int) preferred_class[i]] > 0
 		  && ! small_register_class_p (preferred_class[i]))
 		{
 		  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
-			 common case anyway.  */
-		      if (reg_class_subset_p (preferred_class[i],
-					      this_alternative[i]))
+		      if (reg_classes_intersect_p (preferred_class[i],
+						   this_alternative[i]))
 			this_alternative[i] = preferred_class[i];
 		      else
 			reject += (2 + 2 * pref_or_nothing[i]);
 		    }
-- 
2.30.2

brgds, H-P

Re: [PATCH] reload: Adjust find_reloads to comment; test intersection, not subset

[Richard Sandiford]

Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> I'm not seriously submitting this patch for approval.  I just thought
> it'd be interesting to some people, at least those maintaining ports
> still using reload; I know it's reload and major ports don't really
> care about that anymore.  TL;DR: scroll down for the mystery part.
>
> The *benevolent* effects of this bug had me confused for a while: I
> thought I'd see neutral or good effects of nominally *improving* the
> register-class topology of the CRIS port such that there's "a class
> for the union of two classes whenever some instruction allows both
> classes".  Essentially, IRA picked (still does, not committed yet) two
> classes GENNONACR_REGS and SPECIAL_REGS and SPEC_GENNONACR_REGS, their
> union, but the move-pattern alternatives corresponding to
> GENNONACR_REGS are actually "r" (GENERAL_REGS), which also contains
> the ACR register.  I want to get rid of all things related to ACR and
> simplify the port, as ACR is an artefact of a no-longer-supported CRIS
> variant (see commit r11-220 which also mentions this remaining
> artefact).
>
> But, when improving the topology, instead of still making good use of
> registers from SPECIAL_REGS where the reload pass matters, registers
> from GENERAL_REGS are always preferred.  Though, for coremark, this
> only happens in two printf-formatting functions of newlib; _vfprintf_r
> and _vfiprintf_r (same code actually, and it's likely a bug that both
> instances get included).
>
> Anyway, skipping to the point: the effect of the bug is that
> alternatives corresponding to classes that are *not subclasses* of the
> preferred class (the union), are demoted by 4 (or actually: 2 + 2 *
> pref_or_nothing[i]), instead of those register alternatives where the
> register-class *intersection is empty*, as is the intent according to
> my reading of the comment.  For CRIS, this means that GENERAL_REGS get
> demoted (as ACR is present there but not in SPEC_GENNONACR_REGS) and
> then SPECIAL_REGS are preferred, which is *sometimes* better as those
> registers are "free".  They're not used in regular code and outside
> their special purposes, basically just usable for moving around data;
> perfect for plain single-use spill-reloads.  Net result: for CRIS the
> bug causes a slightly smaller coremark binary but the same execution
> time.
>
> With the CRIS register-class topology fixed and the bug no longer
> having effect, I have to adjust the port such that SPECIAL_REGS are
> preferred when that helps (some heuristics required), as the (current)
> order of alternatives for the *movsi_internal pattern causes
> preference of GENERAL_REGS over SPECIAL_REGS when the cost is the
> same.  IOW, a flaw in the port masked by a bug in reload.
>
> 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 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]?
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.

Thanks,
Richard

> (I know the file has been renamed but again this isn't an patch
> intended to be applied.  The context is adjusted to include the
> comment.  The ChangeLog entry is the effect of a habitual twitch. 8-]
> )
>
> gcc:
> 	* reload.c (find_reloads): Call reg_classes_intersect_p, not
> 	reg_class_subset_p.
> ---
>  gcc/reload.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/reload.c b/gcc/reload.c
> index 4c55ca58a5f7..36e887c6b57e 100644
> --- a/gcc/reload.c
> +++ b/gcc/reload.c
> @@ -3616,43 +3616,39 @@ find_reloads (rtx_insn *insn, int replace, int ind_levels, int live_known,
>  	      /* If this operand is a pseudo register that didn't get
>  		 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
>  		 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
>  		 register in a later insn and avoiding a pair of
>  		 memory stores and loads.
>  
>  		 Don't bother with this if this alternative will
>  		 accept this operand.
>  
>  		 Don't do this for a multiword operand, since it is
>  		 only a small win and has the risk of requiring more
>  		 spill registers, which could cause a large loss.
>  
>  		 Don't do this if the preferred class has only one
>  		 register because we might otherwise exhaust the
>  		 class.  */
>  
>  	      if (! win && ! did_match
>  		  && this_alternative[i] != NO_REGS
>  		  && known_le (GET_MODE_SIZE (operand_mode[i]), UNITS_PER_WORD)
>  		  && reg_class_size [(int) preferred_class[i]] > 0
>  		  && ! small_register_class_p (preferred_class[i]))
>  		{
>  		  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
> -			 common case anyway.  */
> -		      if (reg_class_subset_p (preferred_class[i],
> -					      this_alternative[i]))
> +		      if (reg_classes_intersect_p (preferred_class[i],
> +						   this_alternative[i]))
>  			this_alternative[i] = preferred_class[i];
>  		      else
>  			reject += (2 + 2 * pref_or_nothing[i]);
>  		    }

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

[Hans-Peter Nilsson]

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

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]))
-- 
2.30.2

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

[Richard Sandiford]

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.

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]))

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

[Richard Sandiford]

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]))

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

[Hans-Peter Nilsson]

Rather than assuming it's seen and thought not worth the
bother, I'll go with not-seen, so:

Jeff: ping.  A little love for reload, comment-wise, before it's put down!

> From: Richard Sandiford <richard.sandiford@arm.com>
> CC: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>, "jlaw@tachyum.com"
> 	<jlaw@tachyum.com>
> Date: Wed, 2 Feb 2022 16:16:14 +0100
> Old-Content-Type: multipart/alternative; boundary="_000_mpta6f9fge9fsfarmcom_"
> Content-Type: text/plain; charset=iso-8859-1
> 
> 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.

Perhaps even better to use the address seen in mailing list
conversations, so I'm switching to that one.

> 
> >
> > 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]))
> 

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

[Jeff Law]

On 2/2/2022 8:16 AM, Richard Sandiford via Gcc-patches wrote:
> 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.
:-)  I still got a copy as I'm subscribed to gcc-patches.  I've just 
been incredibly busy since the end of Jan.

I think y'all are right.   Go ahead and commit the comment.   I don't 
think there's an active proposal to remove reload[1].cc before gcc-12, 
but I would hope we can remove it shortly thereafter.

jeff