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 96E833830B1D for ; Thu, 24 Nov 2022 16:12:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 96E833830B1D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com 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 AD14F23A; Thu, 24 Nov 2022 08:12:23 -0800 (PST) Received: from [10.57.5.59] (unknown [10.57.5.59]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6BC963F587; Thu, 24 Nov 2022 08:12:16 -0800 (PST) Message-ID: <585303f9-1f2f-7177-3029-9646dd19f63e@foss.arm.com> Date: Thu, 24 Nov 2022 16:12:15 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [Patch Arm] Fix PR 92999 Content-Language: en-GB To: Ramana Radhakrishnan Cc: gcc-patches , Richard Earnshaw , Alex Coplan References: <4119381e-5b4e-5132-8822-e88f8fbb8fc1@foss.arm.com> From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3489.5 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_NUMSUBJECT,KAM_SHORT,NICE_REPLY_A,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/11/2022 21:50, Ramana Radhakrishnan via Gcc-patches wrote: > On Thu, Nov 10, 2022 at 7:46 PM Ramana Radhakrishnan > wrote: >> >> On Thu, Nov 10, 2022 at 6:03 PM Richard Earnshaw >> wrote: >>> >>> >>> >>> On 10/11/2022 17:21, Richard Earnshaw via Gcc-patches wrote: >>>> >>>> >>>> On 08/11/2022 18:20, Ramana Radhakrishnan via Gcc-patches wrote: >>>>> PR92999 is a case where the VFP calling convention does not allocate >>>>> enough FP registers for a homogenous aggregate containing FP16 values. >>>>> I believe this is the complete fix but would appreciate another set of >>>>> eyes on this. >>>>> >>>>> Could I get a hand with a regression test run on an armhf environment >>>>> while I fix my environment ? >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> PR target/92999 >>>>> * config/arm/arm.c (aapcs_vfp_allocate_return_reg): Adjust to handle >>>>> aggregates with elements smaller than SFmode. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * gcc.target/arm/pr92999.c: New test. >>>>> >>>>> >>>>> Thanks, >>>>> Ramana >>>>> >>>>> Signed-off-by: Ramana Radhakrishnan >>>> >>>> I'm not sure about this. The AAPCS does not mention a base type of a >>>> half-precision FP type as an appropriate homogeneous aggregate for using >>>> VFP registers for either calling or returning. >> >> Ooh interesting, thanks for taking a look and poking at the AAPCS and >> that's a good catch. BF16 should also have the same behaviour as FP16 >> , I suspect ? > > I suspect I got caught out by the definition of the Homogenous > aggregate from Section 5.3.5 > ((https://github.com/ARM-software/abi-aa/blob/2982a9f3b512a5bfdc9e3fea5d3b298f9165c36b/aapcs32/aapcs32.rst#homogeneous-aggregates) > which simply suggests it's an aggregate of fundamental types which > lists half precision floating point . A homogeneous aggregate is any aggregate that fits the general definition, but only HAs of specific types are of interest for the VFP PCS rules. The problem we have is that when we added HFmode (and later BF16mode) support we didn't notice that the base types are VFP candidates, but the nested types (eg in records or arrays) are not. The problems started around SVN r236269 (git:1b81a1c1bd53) when we added FP16 support. > > FTR, ideally I should have read 7.1.2.1 > https://github.com/ARM-software/abi-aa/blob/2982a9f3b512a5bfdc9e3fea5d3b298f9165c36b/aapcs32/aapcs32.rst#procedure-calling) > :) > > > >> >>>> >>>> So perhaps the bug is that we try to treat this as a homogeneous >>>> aggregate at all. >> >> Yep I agree - I'll take a look again tomorrow and see if I can get a fix. >> >> (And thanks Alex for the test run, I might trouble you again while I >> still (slowly) get some of my boards back up) > > > and as promised take 2. I'd really prefer another review on this one > to see if I've not missed anything in the cases below. I think I'd prefer to try and fix this at the point where we accept the base types, ie around: case REAL_TYPE: mode = TYPE_MODE (type); if (mode != DFmode && mode != SFmode && mode != HFmode && mode != BFmode) return -1; by changing this to something like /* HFmode and BFmode can be passed in registers, but are not valid base types for an HFA, so only accept these if we are at the top level. */ if (!(mode == DFmode || mode == SFmode || (depth == 0 && (mode == HFmode || mode == BFmode))) return -1; and we then pass depth into the recursion calls as an extra parameter, starting at 0 for the top level and incrementing it by 1 each time aapcs_vfp_sub_candidate recurses. For the test, would it be possible to rewrite it in the style of gcc.target/arm/aapcs/* and put it there? That would ensure that not only are the caller and callee compatible, but that the values are passed in the correct location. R.