From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 185263858C2C for ; Wed, 2 Feb 2022 15:16:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 185263858C2C Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C6D8A11D4; Wed, 2 Feb 2022 07:16:16 -0800 (PST) Received: from localhost (unknown [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 35ADF3F73B; Wed, 2 Feb 2022 07:16:16 -0800 (PST) From: Richard Sandiford To: Hans-Peter Nilsson Mail-Followup-To: Hans-Peter Nilsson , , jlaw@tachyum.com, richard.sandiford@arm.com Cc: , jlaw@tachyum.com Subject: Re: [PATCH] reload: Adjust comment in find_reloads about subset, not intersection References: <20220201014140.A2D9220419@pchp3.se.axis.com> <20220201200516.EB39120418@pchp3.se.axis.com> Date: Wed, 02 Feb 2022 15:16:14 +0000 In-Reply-To: (Richard Sandiford's message of "Wed, 02 Feb 2022 15:14:43 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, 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: Wed, 02 Feb 2022 15:16:18 -0000 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. > > 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]))