From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp1.axis.com (smtp1.axis.com [195.60.68.17]) by sourceware.org (Postfix) with ESMTPS id CCF233858D3C for ; Mon, 14 Feb 2022 19:28:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CCF233858D3C From: Hans-Peter Nilsson To: CC: , Richard Sandiford In-Reply-To: (message from Richard Sandiford on Wed, 2 Feb 2022 16:16:14 +0100) Subject: Re: [PATCH] reload: Adjust comment in find_reloads about subset, not intersection MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT References: <20220201014140.A2D9220419@pchp3.se.axis.com> <20220201200516.EB39120418@pchp3.se.axis.com> Message-ID: <20220214192809.591D820411@pchp3.se.axis.com> Date: Mon, 14 Feb 2022 20:28:09 +0100 X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Feb 2022 19:28:13 -0000 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 > CC: "gcc-patches@gcc.gnu.org" , "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 writes: > > Hans-Peter Nilsson writes: > >>> From: Richard Sandiford > >>> Hans-Peter Nilsson via Gcc-patches 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])) >