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 2D67A3858C2C for ; Thu, 9 Sep 2021 14:39:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2D67A3858C2C 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 B4D7C6D; Thu, 9 Sep 2021 07:39:02 -0700 (PDT) Received: from [10.57.22.219] (unknown [10.57.22.219]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0E6723F59C; Thu, 9 Sep 2021 07:39:01 -0700 (PDT) Subject: Re: [PATCH v2 1/3] rtl: properly handle subreg (mem) in gen_highpart [PR102125] To: Richard Biener , Richard Earnshaw Cc: GCC Patches References: <20210909110923.3171630-1-rearnsha@arm.com> <20210909110923.3171630-2-rearnsha@arm.com> From: Richard Earnshaw Message-ID: Date: Thu, 9 Sep 2021 15:39:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3498.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, SPF_HELO_NONE, SPF_NONE, TXREP 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: Thu, 09 Sep 2021 14:39:04 -0000 On 09/09/2021 13:23, Richard Biener via Gcc-patches wrote: > On Thu, Sep 9, 2021 at 1:09 PM Richard Earnshaw wrote: >> >> >> gen_lowpart_general handles forming a SUBREG of a MEM by using >> adjust_address to rework and validate a new version of the MEM. >> However, gen_highpart does not attempt this and simply returns (SUBREG >> (MEM)) if the change is not 'obviously' safe. Improve on that by >> using a similar approach so that gen_lowpart and gen_highpart are >> mostly symmetrical in this regard. > > When I decipher gen_lowpart correctly then it doesn't generate the > subreg of the mem in the first place so doing it like that in gen_highpart > would _not_ invoke simplify_gen_subreg on a MEM_P but instead > do what you now do directly? > > I also wonder why gen_lowpart_general uses byte_lowpart_offset > while you use subreg_highpart_offset where subreg_lowpart_offset > is also available ... huh - and there's also > subreg_size_{lowpart,highpart}_offset. > So it looks like your case wouldn't handle the paradoxical highpart > (which better shouldn't be accessed?). > Surely the highpart of a paradoxical subreg is meaningless... what's the highpart when the outer subreg is wider than the inner one? And that's why there is subreg_lowpart_offset, subreg_highpart_offset and byte_lowpart_offset, but not byte_highpart_offset (because the latter is there to handle paradoxical cases, but decays to subreg_lowpart_offset for a non-paradoxical subreg case). > So like > > diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c > index 77ea8948ee8..c3dae7d8075 100644 > --- a/gcc/emit-rtl.c > +++ b/gcc/emit-rtl.c > @@ -1585,6 +1585,13 @@ gen_highpart (machine_mode mode, rtx x) > gcc_assert (known_le (msize, (unsigned int) UNITS_PER_WORD) > || known_eq (msize, GET_MODE_UNIT_SIZE (GET_MODE (x)))); > > + /* Offset MEMs. */ > + if (MEM_P (x)) > + { > + poly_int64 offset = subreg_highpart_offset (mode, GET_MODE (x)); > + return adjust_address (x, mode, offset); > + } > + > result = simplify_gen_subreg (mode, x, GET_MODE (x), > subreg_highpart_offset (mode, GET_MODE (x))); > gcc_assert (result); > In which case, I'm pretty certain the subsequent MEM_P (result) test can be removed, as I can't see how simplify_gen_subreg would return a MEM with such a change. > Testing > > + else if (GET_CODE (result) == SUBREG && MEM_P (SUBREG_REG (result)) > + && MEM_P (x)) > > looks a bit odd to me. > > I'll note it leaves gen_highpart_mode "unfixed", some refactoring should > instead commonize the worker for both interfaces, making gen_highpart > invoke gen_highpart_mode or so. > gen_highpart_mode invokes gen_highpart if the inner mode is not VOIDmode. Perhaps the logic is somewhat backwards, or perhaps it's just a bit more efficient that way. I'll try your suggested change. R. >> gcc/ChangeLog: >> >> PR target/102125 >> * emit-rtl.c (gen_highpart): If simplify_gen_subreg returns >> SUBREG (MEM) for a MEM, use adjust_address to produce a new >> MEM. >> --- >> gcc/emit-rtl.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >>