From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113790 invoked by alias); 25 Jan 2016 16:38:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 113772 invoked by uid 89); 25 Jan 2016 16:38:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=popping, sk:reimple, 1,24, validations X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 25 Jan 2016 16:37:59 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0E1AB3FB; Mon, 25 Jan 2016 08:37:16 -0800 (PST) Received: from [10.2.206.200] (e100706-lin.cambridge.arm.com [10.2.206.200]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C5D963F246; Mon, 25 Jan 2016 08:37:55 -0800 (PST) Message-ID: <56A64F62.6040500@foss.arm.com> Date: Mon, 25 Jan 2016 16:38:00 -0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Christian Bruel , "ramana.gcc@googlemail.com" , "Richard.Earnshaw@arm.com" CC: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function References: <56A0B4C7.5090609@st.com> <56A0CD87.5050108@foss.arm.com> <56A237B4.60407@st.com> <56A239F6.30102@foss.arm.com> <56A241E1.6090901@st.com> In-Reply-To: <56A241E1.6090901@st.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2016-01/txt/msg01899.txt.bz2 On 22/01/16 14:51, Christian Bruel wrote: > Hi Kyrill, > > On 01/22/2016 03:17 PM, Kyrill Tkachov wrote: >> Hi Christian, >> >> On 22/01/16 14:07, Christian Bruel wrote: >>> Hi Kyrill, >>> >>> On 01/21/2016 01:22 PM, Kyrill Tkachov wrote: >>>> Hi Christian, >>>> >>>> On 21/01/16 10:36, Christian Bruel wrote: >>>>> The current arm_set_current_function was both awkward and buggy. For instance using partially set TARGET_OPTION set from pragma_parse, while restore_target_globalsnor arm_option_params_internal was not reset. Another issue is that in >>>>> some >>>>> paths, target_reinit was not called due to old cached target_option_current_node value. for instance with >>>>> >>>>> foo{} >>>>> #pragma GCC target ... >>>>> >>>>> foo was called with global_options set from old GCC target (which was wrong) and correct rtl values. >>>>> >>>>> This is a reimplementation of the function. Hoping to be easier to read (and maintain). Solves the current issues seen so far. >>>>> >>>>> regtested for arm-linux-gnueabi -mfpu=vfp, -mfpu=neon,-mfpu=neon-fp-armv8 >>>>> >>>> Thanks for the patch, I'll try it out. >>>> In the meantime there's a couple of style and typo nits... >>>> >>>> + /* Make sure that target_reinit is called for next function, since >>>> + TREE_TARGET_OPTION might change with the #pragma even if there are >>>> + no target attribute attached to the function. */ >>>> >>>> s/attribute/attributes >>>> >>>> - arm_previous_fndecl = fndecl; >>>> + /* if no attribute, use the mode set by the current pragma target. */ >>>> + if (! new_tree) >>>> + new_tree = target_option_current_node; >>>> + >>>> >>>> s/if/If/ >>>> >>>> + /* now target_reinit can save the state for later. */ >>>> + TREE_TARGET_GLOBALS (new_tree) >>>> + = save_target_globals_default_opts (); >>>> >>>> s/now/Now/ >>>> >>> While playing on my side. I realized that we could simplify the patch further by removing the need to set and use target_option_current_node, since this is redundant with what handle_pragma_push/pop_options does. >>> Also since that the functions inside a pragma GCC target region will have DECL_FUNCTION_SPECIFIC_TARGET set already, we don't seem to need a special case for those. >>> >>> With this V2, arm_set_current_function is becoming more minimalist and still fixes the current issues. Could you test this version instead ? >>> >> Thanks, I'll check this out instead. >> I've played a bit with your previous version and the effect on the testcases looked ok, but I have a couple of >> comments on the testcase in the meantime >> >> Index: gcc/testsuite/gcc.target/arm/pr69245.c >> =================================================================== >> --- gcc/testsuite/gcc.target/arm/pr69245.c (revision 0) >> +++ gcc/testsuite/gcc.target/arm/pr69245.c (working copy) >> @@ -0,0 +1,24 @@ >> +/* PR target/69245 */ >> +/* Test that pop_options restores the vfp fpu mode. */ >> +/* { dg-do compile } */ >> +/* { dg-require-effective-target arm_fp_ok } */ >> +/* { dg-add-options arm_fp } */ >> + >> +#pragma GCC target "fpu=vfp" >> + >> +#pragma GCC push_options >> +#pragma GCC target "fpu=neon" >> +int a, c, d; >> +float b; >> +static int fn1 () >> +{ >> + return 0; >> +} >> +#pragma GCC pop_options >> + >> +void fn2 () >> +{ >> + d = b * c + a; >> +} >> + >> +/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */ >> >> >> PR 69245 is an ICE whereas your testcase doesn't ICE without the patch, it just fails the >> scan-assembler check. I'd like to have the testcase trigger the ICE without your patch. >> For that we need -O2 in dg-options. >> Also, the "fpu=vfp" pragma you put in the beginning doesn't allow the ICE to trigger, presumably >> because it triggers a different path through the pragma option popping code. >> So removing that pragma and instead changing the dg-add-options from arm_fp to arm_vfp3 (which is >> floating-point without the vfma instruction causes the ICE) does the trick for me. >> Also the "fpu=neon" pragma should also be changed to be "fpu=neon-vfpv4" because that setting allows >> the vfma instruction which is being wrongly considered in fn2(). >> I suppose you'll then want to change the scan-assembler directive to look for \.fpu vfp3. > > ah yes ! OK for -O2, I thought I had it, must have been deleted somewhere :-( > > I added the #pragma GCC target "fpu=vfp" to have some kind of deterministic checks to guard against the options permutations that Christophe stresses during his validations. so for instance the ".fpu scan-assembler would change depending > on the default options... > > so the following test should ICE with the all configurations (!-mfloat-abi=soft) in -O2 > > #pragma GCC target "fpu=vfp" > > #pragma GCC push_options > #pragma GCC target "fpu=neon-vfpv4" > int a, c, d; > float b; > static int fn1 () > { > return 0; > } > > #pragma GCC pop_options > void fn2 () > { > d = b * c + a; > } > > Ah ok, I needed to update my tree to include your other midend fixes in this area. I played around with the patch and gave it a bootstrap as well. I wanted to make a sanity check on compile-time performance for files using arm_neon.h and I didn't spot any measurable regressions. So this is ok for trunk with the testcase changed as discussed above and using -O2 optimisation level and with a couple comment fixes below. - arm_previous_fndecl = fndecl; + /* if no attribute, but there was one. Use the default mode. */ + if (! new_tree && old_tree) + new_tree = target_option_default_node; + Start with capital 'I'. Maybe phrase it as: "If current function has no attributes but previous one did, use the default node." ? + /* Call target_reinit and save the state for TARGET_GLOBALS. */ + TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts (); Two spaces between full stop and comment closure. Thanks, Kyrill