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.sandif...@arm.com>
> CC: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>, "j...@tachyum.com"
> <j...@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.sandif...@arm.com> writes:
> > Hans-Peter Nilsson <h...@axis.com> writes:
> >>> From: Richard Sandiford <richard.sandif...@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]))
>
