From patchwork Tue Jan 26 15:29:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Bruel X-Patchwork-Id: 573340 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id A7DE1140BAE for ; Wed, 27 Jan 2016 02:30:12 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=qPPKGa8h; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=lq87BDhZiymo3rz34 wd3kASKSBdMRceqbcM0g1yj2vPSPQbi7udU0ZPOZR0Pe1lgewSj0EuXaSFqvL335 92eFJBRFcdvGUhjDMyKJJubpeCJHX9SSEQwo2CWVYzWIbnT/2Lc0JBtKZeobuNAj RMUFv1PtrDVvaWeWyoE73rKNKo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=cRmwJrb7yObhRDRaGLEmOiJ +I30=; b=qPPKGa8hu2ai3+mSzFVkm47/dXQxL9rfBGTiqo3MJW3lew6x+0kEYyb 6gBo73KgwrgMkgxB4dqhyg/6gyWoWZM3Wtyc8wmXQpMTY0wtio2WPJPIixshTNW6 uRBa3Fk7xdwClcWpo2rDSH2JsdAFrameyql2tdoZ1DTvaX32ixJ0= Received: (qmail 126752 invoked by alias); 26 Jan 2016 15:30:02 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 124835 invoked by uid 89); 26 Jan 2016 15:30:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.9 required=5.0 tests=BAYES_50, KAM_ASCII_DIVIDERS, KAM_LAZY_DOMAIN_SECURITY, KHOP_DYNAMIC, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=2216, neonvfpv4, arm-c.c, UD:arm-c.c X-HELO: mx07-00178001.pphosted.com Received: from mx08-00178001.pphosted.com (HELO mx07-00178001.pphosted.com) (91.207.212.93) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 26 Jan 2016 15:30:00 +0000 Received: from pps.filterd (m0046660.ppops.net [127.0.0.1]) by mx08-00178001.pphosted.com (8.15.0.59/8.15.0.59) with SMTP id u0QFNbsa013136; Tue, 26 Jan 2016 16:29:53 +0100 Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx08-00178001.pphosted.com with ESMTP id 20m4jgjd2t-1 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-SHA bits=256 verify=NOT); Tue, 26 Jan 2016 16:29:53 +0100 Received: from zeta.dmz-eu.st.com (zeta.dmz-eu.st.com [164.129.230.9]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 421A731; Tue, 26 Jan 2016 15:29:01 +0000 (GMT) Received: from Webmail-eu.st.com (safex1hubcas4.st.com [10.75.90.69]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 5574D2666; Tue, 26 Jan 2016 15:29:50 +0000 (GMT) Received: from [164.129.122.197] (164.129.122.197) by webmail-eu.st.com (10.75.90.13) with Microsoft SMTP Server (TLS) id 8.3.389.2; Tue, 26 Jan 2016 16:29:49 +0100 Subject: Re: [PATCH][ARM] Fix PR target/69245 Rewrite arm_set_current_function To: Kyrill Tkachov , "ramana.gcc@googlemail.com" , "Richard.Earnshaw@arm.com" References: <56A0B4C7.5090609@st.com> <56A0CD87.5050108@foss.arm.com> <56A237B4.60407@st.com> <56A239F6.30102@foss.arm.com> <56A241E1.6090901@st.com> <56A64F62.6040500@foss.arm.com> CC: "gcc-patches@gcc.gnu.org" From: Christian Bruel X-No-Archive: yes Message-ID: <56A790ED.60603@st.com> Date: Tue, 26 Jan 2016 16:29:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56A64F62.6040500@foss.arm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-01-26_05:, , signatures=0 X-IsSubscribed: yes On 01/25/2016 05:37 PM, Kyrill Tkachov wrote: 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. Hi Kyrill, I realized afterwards that my implementation had a flaw with the handling of #pragma GCC reset. What happened is that when both old and new TREE_TARGET_OPTION are NULL, we didn't save_restore the globals flags, to save compute time. The problem is that with #pragma GCC reset, we also fall into this situation, and exit without calling target_reeinit :-( Handling this situation doesn't complicate the code much, because I factorized the calls to target_reeinit + restore_target_globals into a new function (save_restore_target_globals). This function is called from the pragma context when resetting the state arm_reset_previous_fndecl to the default, and from arm_set_current_function when setting to a new target. This is only done when there is a change of the target flags, so no extra computing cost. Same testing as with previous patch: arm-qemu/ arm-qemu//-mfpu=neon-fp-armv8 arm-qemu//-mfpu=neon Still OK ? 2016-01-20 Christian Bruel PR target/69245 * config/arm/arm-c.c (arm_pragma_target_parse): Add comments. Move arm_reset_previous_fndecl and set_target_option_current_node in the conditional part. Call save_restore_target_globals. * config/arm/arm.c (arm_set_current_function): Refactor to better support #pragma target and attribute mix. Call save_restore_target_globals. * config/arm/arm-protos.h (arm_reset_previous_fndecl): Add parameter. (save_restore_target_globals): New function. 2016-01-20 Christian Bruel PR target/69245 * gcc.target/arm/pr69245.c: New test. Index: gcc/config/arm/arm-c.c =================================================================== --- gcc/config/arm/arm-c.c (revision 232831) +++ gcc/config/arm/arm-c.c (working copy) @@ -221,9 +221,6 @@ arm_pragma_target_parse (tree args, tree pop_targe } } - target_option_current_node = cur_tree; - arm_reset_previous_fndecl (); - /* Figure out the previous mode. */ prev_opt = TREE_TARGET_OPTION (prev_tree); cur_opt = TREE_TARGET_OPTION (cur_tree); @@ -259,6 +256,13 @@ arm_pragma_target_parse (tree args, tree pop_targe arm_cpu_builtins (parse_in); cpp_opts->warn_unused_macros = saved_warn_unused_macros; + + /* Make sure that target_reinit is called for next function, since + TREE_TARGET_OPTION might change with the #pragma even if there is + no target attribute attached to the function. */ + if (cur_tree != target_option_default_node) + cur_tree = NULL; + arm_reset_previous_fndecl (cur_tree); } return true; Index: gcc/config/arm/arm-protos.h =================================================================== --- gcc/config/arm/arm-protos.h (revision 232831) +++ gcc/config/arm/arm-protos.h (working copy) @@ -331,7 +331,7 @@ extern bool arm_autoinc_modes_ok_p (machine_mode, extern void arm_emit_eabi_attribute (const char *, int, int); -extern void arm_reset_previous_fndecl (void); +extern void arm_reset_previous_fndecl (tree); /* Defined in gcc/common/config/arm-common.c. */ extern const char *arm_rewrite_selected_cpu (const char *name); Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c (revision 232831) +++ gcc/config/arm/arm.c (working copy) @@ -3446,8 +3446,7 @@ arm_option_override (void) /* Save the initial options in case the user does function specific options. */ - target_option_default_node = target_option_current_node - = build_target_option_node (&global_options); + target_option_default_node = build_target_option_node (&global_options); /* Init initial mode for testing. */ thumb_flipper = TARGET_THUMB; @@ -29746,10 +29745,31 @@ arm_is_constant_pool_ref (rtx x) /* Remember the last target of arm_set_current_function. */ static GTY(()) tree arm_previous_fndecl; +/* Restore the TREE_TARGET_GLOBALS from previous state, or save it. */ +static void +save_restore_target_globals (tree new_tree) +{ + /* If we have a previous state, use it. */ + if (TREE_TARGET_GLOBALS (new_tree)) + restore_target_globals (TREE_TARGET_GLOBALS (new_tree)); + else if (new_tree == target_option_default_node) + restore_target_globals (&default_target_globals); + else + { + /* Call target_reinit and save the state for TARGET_GLOBALS. */ + TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts (); + } + + arm_option_params_internal (); +} + /* Invalidate arm_previous_fndecl. */ void -arm_reset_previous_fndecl (void) +arm_reset_previous_fndecl (tree new_tree) { + if (new_tree) + save_restore_target_globals (new_tree); + arm_previous_fndecl = NULL_TREE; } @@ -29768,38 +29788,23 @@ arm_set_current_function (tree fndecl) tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl); - arm_previous_fndecl = fndecl; + /* If current function has no attributes but previous one did, + use the default node." */ + if (! new_tree && old_tree) + new_tree = target_option_default_node; + + /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to + the default have been handled by save_restore_target_globals from + arm_pragma_target_parse. */ if (old_tree == new_tree) return; - if (new_tree && new_tree != target_option_default_node) - { - cl_target_option_restore (&global_options, - TREE_TARGET_OPTION (new_tree)); + arm_previous_fndecl = fndecl; - if (TREE_TARGET_GLOBALS (new_tree)) - restore_target_globals (TREE_TARGET_GLOBALS (new_tree)); - else - TREE_TARGET_GLOBALS (new_tree) - = save_target_globals_default_opts (); - } + /* First set the target options. */ + cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree)); - else if (old_tree && old_tree != target_option_default_node) - { - new_tree = target_option_current_node; - - cl_target_option_restore (&global_options, - TREE_TARGET_OPTION (new_tree)); - if (TREE_TARGET_GLOBALS (new_tree)) - restore_target_globals (TREE_TARGET_GLOBALS (new_tree)); - else if (new_tree == target_option_default_node) - restore_target_globals (&default_target_globals); - else - TREE_TARGET_GLOBALS (new_tree) - = save_target_globals_default_opts (); - } - - arm_option_params_internal (); + save_restore_target_globals (new_tree); } /* Implement TARGET_OPTION_PRINT. */ 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,26 @@ +/* PR target/69245 */ +/* Test that pop_options restores the vfp fpu mode. */ +/* { dg-do compile } */ +/* { dg-require-effective-target arm_neon_ok } */ +/* { dg-require-effective-target arm_fp_ok } */ +/* { dg-options "-O2" } */ +/* { dg-add-options arm_fp } */ + +#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; +} + +/* { dg-final { scan-assembler-times "\.fpu vfp" 1 } } */