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 45979393BC02 for ; Wed, 19 May 2021 14:45:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 45979393BC02 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 E659A31B; Wed, 19 May 2021 07:45:45 -0700 (PDT) Received: from [10.57.61.192] (unknown [10.57.61.192]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1D6CF3F73B; Wed, 19 May 2021 07:45:44 -0700 (PDT) Subject: Re: [PATCH] arm: Fix ICE with CMSE nonsecure call on Armv8.1-M [PR100333] To: Alex Coplan Cc: Richard Earnshaw , gcc-patches@gcc.gnu.org, Ramana Radhakrishnan References: <20210430083016.exw25bd45jkktq7p@arm.com> <20210519144200.cjaiigk5dxdjxa4f@arm.com> From: Richard Earnshaw Message-ID: <4e52eac7-7991-4da6-b092-e519e8571d36@foss.arm.com> Date: Wed, 19 May 2021 15:45:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210519144200.cjaiigk5dxdjxa4f@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3491.9 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 19 May 2021 14:45:47 -0000 ENOATTACHMENT. On 19/05/2021 15:42, Alex Coplan via Gcc-patches wrote: > Hi Richard, > > On 17/05/2021 17:31, Richard Earnshaw wrote: >> >> >> On 30/04/2021 09:30, Alex Coplan via Gcc-patches wrote: >>> Hi, >>> >>> As the PR shows, we ICE shortly after expanding nonsecure calls for >>> Armv8.1-M. For Armv8.1-M, we have TARGET_HAVE_FPCXT_CMSE. As it stands, >>> the expander (arm.md:nonsecure_call_internal) moves the callee's address >>> to a register (with copy_to_suggested_reg) only if >>> !TARGET_HAVE_FPCXT_CMSE. >>> >>> However, looking at the pattern which the insn appears to be intended to >>> match (thumb2.md:*nonsecure_call_reg_thumb2_fpcxt), it requires the >>> callee's address to be in a register. >>> >>> This patch therefore just forces the callee's address into a register in >>> the expander. >>> >>> Testing: >>> * Regtested an arm-eabi cross configured with >>> --with-arch=armv8.1-m.main+mve.fp+fp.dp --with-float=hard. No regressions. >>> * Bootstrap and regtest on arm-linux-gnueabihf in progress. >>> >>> OK for trunk and backports as appropriate if bootstrap looks good? >>> >>> Thanks, >>> Alex >>> >>> gcc/ChangeLog: >>> >>> PR target/100333 >>> * config/arm/arm.md (nonsecure_call_internal): Always ensure >>> callee's address is in a register. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR target/100333 >>> * gcc.target/arm/cmse/pr100333.c: New test. >>> >> >> >> - " >> { >> - if (!TARGET_HAVE_FPCXT_CMSE) >> - { >> - rtx tmp = >> - copy_to_suggested_reg (XEXP (operands[0], 0), >> - gen_rtx_REG (SImode, R4_REGNUM), >> - SImode); >> + rtx tmp = NULL_RTX; >> + rtx addr = XEXP (operands[0], 0); >> >> - operands[0] = replace_equiv_address (operands[0], tmp); >> - } >> - }") >> + if (TARGET_HAVE_FPCXT_CMSE && !REG_P (addr)) >> + tmp = force_reg (SImode, addr); >> + else if (!TARGET_HAVE_FPCXT_CMSE) >> + tmp = copy_to_suggested_reg (XEXP (operands[0], 0), >> + gen_rtx_REG (SImode, R4_REGNUM), >> + SImode); >> >> >> I think it might be better to handle the !TARGET_HAVE_FPCXT_CMSE case via a >> pseudo as well, then we don't end up generating a potentially non-trivial >> insn that directly writes a fixed hard reg - it's better to let later passes >> clean that up if they can. > > Ah, I wasn't aware that was an issue. > >> >> Also, you've extracted XEXP (operands[0], 0) into 'addr', but then continue >> to use the XEXP form in the existing path. Please be consistent use XEXP >> directly everywhere, or use 'addr' everywhere. > > Fixed, thanks. > >> >> So you want something like >> >> addr = XEXP (operands[0], 0); >> if (!REG_P (addr)) >> addr = force_reg (SImode, addr); >> >> if (!T_H_F_C) >> addr = copy...(addr, gen(r4), SImode); >> >> operands[0] = replace_equiv_addr (operands[0], addr); >> >> R. > > How about the attached? Regtested an armv8.1-m.main cross, bootstrapped/regtested > on arm-linux-gnueabihf: no issues. > > OK for trunk and eventual backports? > > Thanks, > Alex >