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 C23813858C54 for ; Tue, 23 Jan 2024 12:14:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C23813858C54 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C23813858C54 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706012072; cv=none; b=ksca4ItRDeQpEufCVErmVyzPs4nkjhDdxvZ3dZqZrocQa4UMjolazYo0NJS4xvkHy476e8OvGhyx9ZkGnmc+n9kpr6x8kv1/RZ3UJbVMpEARfpvrkyayi/JxkAoO7666P2o20R7Nd8sIfDXsvFZgvIGp7QIVOtpidFXs09mai0w= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706012072; c=relaxed/simple; bh=7yA8dPA5QMMHFkHpqsOsdUiEM3huIoMUVkAKicZ9HSM=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=xUz4UfPjHcdb99Jm0fxFXvd5metBEpgOASVFDEH1qG2a/Jdm/OE7LWp05ZXBU3Als5AoJKhMbDeoTqVBBpiPN/qPfXGaeEjlJ7par1op+mfIU2sm0tOUkOJfPawr2jYVXqH8zp9wKroNj2f8JHNPh7SI/1hl9Tz3cE1vKMlwDXk= ARC-Authentication-Results: i=1; server2.sourceware.org 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 9FF5B1FB; Tue, 23 Jan 2024 04:15:14 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 919A03F762; Tue, 23 Jan 2024 04:14:28 -0800 (PST) From: Richard Sandiford To: Alex Coplan Mail-Followup-To: Alex Coplan ,gcc-patches@gcc.gnu.org, Kyrylo Tkachov , Richard Earnshaw , richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, Kyrylo Tkachov , Richard Earnshaw Subject: Re: [PATCH 3/4] rtl-ssa: Ensure new defs get inserted [PR113070] In-Reply-To: (Alex Coplan's message of "Mon, 22 Jan 2024 21:44:19 +0000") References: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) Date: Tue, 23 Jan 2024 12:14:27 +0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-21.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Alex Coplan writes: > On 22/01/2024 13:49, Richard Sandiford wrote: >> Alex Coplan writes: >> > In r14-5820-ga49befbd2c783e751dc2110b544fe540eb7e33eb I added support to >> > RTL-SSA for inserting new insns, which included support for users >> > creating new defs. >> > >> > However, I missed that apply_changes_to_insn needed updating to ensure >> > that the new defs actually got inserted into the main def chain. This >> > meant that when the aarch64 ldp/stp pass inserted a new stp insn, the >> > stp would just get skipped over during subsequent alias analysis, as its >> > def never got inserted into the memory def chain. This (unsurprisingly) >> > led to wrong code. >> > >> > This patch fixes the issue by ensuring new user-created defs get >> > inserted. I would have preferred to have used a flag internal to the >> > defs instead of a separate data structure to keep track of them, but since >> > machine_mode increased to 16 bits we're already at 64 bits in access_info, >> > and we can't really reuse m_is_temp as the logic in finalize_new_accesses >> > requires it to get cleared. >> > >> > Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk? >> > >> > Thanks, >> > Alex >> > >> > gcc/ChangeLog: >> > >> > PR target/113070 >> > * rtl-ssa.h: Include hash-set.h. >> > * rtl-ssa/changes.cc (function_info::finalize_new_accesses): Add >> > new_sets parameter and use it to keep track of new user-created sets. >> > (function_info::apply_changes_to_insn): Also call add_def on new sets. >> > (function_info::change_insns): Add hash_set to keep track of new >> > user-created defs. Plumb it through. >> > * rtl-ssa/functions.h: Add hash_set parameter to finalize_new_accesses and >> > apply_changes_to_insn. >> > --- >> > gcc/rtl-ssa.h | 1 + >> > gcc/rtl-ssa/changes.cc | 28 +++++++++++++++++++++------- >> > gcc/rtl-ssa/functions.h | 6 ++++-- >> > 3 files changed, 26 insertions(+), 9 deletions(-) >> > >> > diff --git a/gcc/rtl-ssa.h b/gcc/rtl-ssa.h >> > index f0cf656f5ac..17337639ae8 100644 >> > --- a/gcc/rtl-ssa.h >> > +++ b/gcc/rtl-ssa.h >> > @@ -50,6 +50,7 @@ >> > #include "mux-utils.h" >> > #include "rtlanal.h" >> > #include "cfgbuild.h" >> > +#include "hash-set.h" >> > >> > // Provides the global crtl->ssa. >> > #include "memmodel.h" >> > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc >> > index ce51d6ccd8d..6119ec3535b 100644 >> > --- a/gcc/rtl-ssa/changes.cc >> > +++ b/gcc/rtl-ssa/changes.cc >> > @@ -429,7 +429,8 @@ update_insn_in_place (insn_change &change) >> > // POS gives the final position of INSN, which hasn't yet been moved into >> > // place. >> >> The new parameter should be documented. How about: >> >> // place. NEW_SETS contains the new set_infos that are being added as part >> // of this change (as opposed to being moved or repurposed from existing >> // instructions). > > That comment looks appropriate for apply_changes_to_insn, where NEW_SETS has > already been populated, but doesn't seem accurate for finalize_new_accesses. > How about: > > // place. Keep track of any newly-created set_infos being added as > // part of this change by adding them to NEW_SETS. > > for finalize_new_accesses? OK with that change (and using your suggestion for > apply_changes_to_insn)? Yeah, sounds good to me. Richard