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