From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTPS id 8B5A83858D39; Wed, 2 Mar 2022 12:25:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8B5A83858D39 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5E39A1166A4; Wed, 2 Mar 2022 07:25:10 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3QdVtkHCWDOL; Wed, 2 Mar 2022 07:25:10 -0500 (EST) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id C805711669B; Wed, 2 Mar 2022 07:25:09 -0500 (EST) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 222CP0KO896673 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 2 Mar 2022 09:25:01 -0300 From: Alexandre Oliva To: Richard Biener Cc: Jeff Law , Vladimir Makarov , zsojka@seznam.cz, GCC Patches , wilson@gcc.gnu.org Subject: Re: [PR103302] skip multi-word pre-move clobber during lra Organization: Free thinker, does not speak for AdaCore References: <74d2af86-c1c4-0842-b319-09e76b3bf633@gmail.com> <4bf89538-8c3c-59a7-378b-d02982e6a5ea@gmail.com> <03226f85-0334-1840-94f9-6c618175f974@gmail.com> Errors-To: aoliva@lxoliva.fsfla.org Date: Wed, 02 Mar 2022 09:25:00 -0300 In-Reply-To: (Alexandre Oliva's message of "Tue, 01 Mar 2022 17:15:45 -0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.2 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 Mar 2022 12:25:12 -0000 On Mar 1, 2022, Alexandre Oliva wrote: > On Feb 23, 2022, Alexandre Oliva wrote: >> On Feb 21, 2022, Richard Biener wrote: >>>> Ok to revert commit r12-5852-g50e8b0c9bca6cdc57804f860ec5311b641753fbb >>> OK. Please re-open the bug as appropriate. >> Thanks. I've reopened it. Here's what I'm installing. I'm not >> reverting the testcase, since it stopped failing even before the patch >> was put in. > And now here's a patch that fixes the underlying issue. Oops, I missed the important question: > Undo multi-word optional reloads correctly > Unlike e.g. remove_inheritance_pseudos, undo_optional_reloads didn't > deal with subregs, so instead of removing multi-word moves, it > replaced the reload pseudo with the original pseudo. Besides the > redundant move, that retained the clobber of the dest, that starts a > multi-word move. After the remap, the sequence that should have > become a no-op move starts by clobbering the original pseudo and then > moving its pieces onto themselves. The problem is the clobber: it > makes earlier sets of the original pseudo to be regarded as dead: if > the optional reload sequence was an output reload, the insn for which > the output reload was attempted may be regarded as dead and deleted. > I've arranged for undo_optional_reloads to accept SUBREGs and use > get_regno, like remove_inheritance_pseudo, adjusted its insn-removal > loop to tolerate iterating over a removed clobber, and added logic to > catch any left-over reload clobbers that could trigger the problem. Regstrapped on x86_64-linux-gnu, also tested on various riscv and arm targets (with gcc-11). Ok to install? > for gcc/ChangeLog > * lra-constraints.cc (undo_optional_reloads): Recognize and > drop insns of multi-word move sequences, tolerate removal > iteration on an already-removed clobber, and refuse to > substitute original pseudos into clobbers. > --- > gcc/lra-constraints.cc | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc > index b2c4590153c4c..5cd89e7eeddc2 100644 > --- a/gcc/lra-constraints.cc > +++ b/gcc/lra-constraints.cc > @@ -7261,15 +7261,17 @@ undo_optional_reloads (void) > continue; > src = SET_SRC (set); > dest = SET_DEST (set); > - if (! REG_P (src) || ! REG_P (dest)) > + if ((! REG_P (src) && ! SUBREG_P (src)) > + || (! REG_P (dest) && ! SUBREG_P (dest))) > continue; > - if (REGNO (dest) == regno > + if (get_regno (dest) == (int) regno > /* Ignore insn for optional reloads itself. */ > - && REGNO (lra_reg_info[regno].restore_rtx) != REGNO (src) > + && (get_regno (lra_reg_info[regno].restore_rtx) > + != get_regno (src)) > /* Check only inheritance on last inheritance pass. */ > - && (int) REGNO (src) >= new_regno_start > + && get_regno (src) >= new_regno_start > /* Check that the optional reload was inherited. */ > - && bitmap_bit_p (&lra_inheritance_pseudos, REGNO (src))) > + && bitmap_bit_p (&lra_inheritance_pseudos, get_regno (src))) > { > keep_p = true; > break; > @@ -7291,18 +7293,22 @@ undo_optional_reloads (void) > bitmap_copy (insn_bitmap, &lra_reg_info[regno].insn_bitmap); > EXECUTE_IF_SET_IN_BITMAP (insn_bitmap, 0, uid, bi2) > { > + /* We may have already removed a clobber. */ > + if (!lra_insn_recog_data[uid]) > + continue; > insn = lra_insn_recog_data[uid]->insn; > if ((set = single_set (insn)) != NULL_RTX) > { > src = SET_SRC (set); > dest = SET_DEST (set); > - if (REG_P (src) && REG_P (dest) > - && ((REGNO (src) == regno > - && (REGNO (lra_reg_info[regno].restore_rtx) > - == REGNO (dest))) > - || (REGNO (dest) == regno > - && (REGNO (lra_reg_info[regno].restore_rtx) > - == REGNO (src))))) > + if ((REG_P (src) || SUBREG_P (src)) > + && (REG_P (dest) || SUBREG_P (dest)) > + && ((get_regno (src) == (int) regno > + && (get_regno (lra_reg_info[regno].restore_rtx) > + == get_regno (dest))) > + || (get_regno (dest) == (int) regno > + && (get_regno (lra_reg_info[regno].restore_rtx) > + == get_regno (src))))) > { > if (lra_dump_file != NULL) > { > @@ -7310,7 +7316,7 @@ undo_optional_reloads (void) > INSN_UID (insn)); > dump_insn_slim (lra_dump_file, insn); > } > - delete_move_and_clobber (insn, REGNO (dest)); > + delete_move_and_clobber (insn, get_regno (dest)); > continue; > } > /* We should not worry about generation memory-memory > @@ -7319,6 +7325,11 @@ undo_optional_reloads (void) > we remove the inheritance pseudo and the optional > reload. */ > } > + if (GET_CODE (PATTERN (insn)) == CLOBBER > + && REG_P (SET_DEST (insn)) > + && get_regno (SET_DEST (insn)) == (int) regno) > + /* Refuse to remap clobbers to preexisting pseudos. */ > + gcc_unreachable (); > lra_substitute_pseudo_within_insn > (insn, regno, lra_reg_info[regno].restore_rtx, false); > lra_update_insn_regno_info (insn); -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about