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 BC90C3857820 for ; Tue, 1 Feb 2022 20:05:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BC90C3857820 From: Hans-Peter Nilsson To: Richard Sandiford CC: In-Reply-To: (message from Richard Sandiford on Tue, 1 Feb 2022 04:57:17 +0100) Subject: [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> Message-ID: <20220201200516.EB39120418@pchp3.se.axis.com> Date: Tue, 1 Feb 2022 21:05:16 +0100 X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Tue, 01 Feb 2022 20:05:20 -0000 > 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. 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