From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1257 invoked by alias); 4 Sep 2015 11:19:50 -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 1245 invoked by uid 89); 4 Sep 2015 11:19:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mx07-00178001.pphosted.com Received: from mx07-00178001.pphosted.com (HELO mx07-00178001.pphosted.com) (62.209.51.94) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 04 Sep 2015 11:19:48 +0000 Received: from pps.filterd (m0046668.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.14.5/8.14.5) with SMTP id t84BI9rq024805; Fri, 4 Sep 2015 13:19:43 +0200 Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com with ESMTP id 1wpg2q7eus-1 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Fri, 04 Sep 2015 13:19:43 +0200 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 1017A31; Fri, 4 Sep 2015 11:19:30 +0000 (GMT) Received: from Webmail-eu.st.com (safex1hubcas6.st.com [10.75.90.73]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 0A57E5AC1; Fri, 4 Sep 2015 11:19:42 +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; Fri, 4 Sep 2015 13:19:41 +0200 Subject: Re: [PATCH, ARM]: Add TARGET_OPTION[RESTORE,SAVE,PRINT] hooks To: Kyrill Tkachov , Ramana Radhakrishnan References: <55E6BD35.5020601@st.com> <55E9644E.2080708@arm.com> CC: "gcc-patches@gcc.gnu.org" From: Christian Bruel X-No-Archive: yes Message-ID: <55E97E4D.307@st.com> Date: Fri, 04 Sep 2015 11:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <55E9644E.2080708@arm.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-09-04_06:2015-09-04,2015-09-04,1970-01-01 signatures=0 X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00340.txt.bz2 Hi Kyrill, On 09/04/2015 11:28 AM, Kyrill Tkachov wrote: > Hi Christian, > > Thanks again for working on this! > > On 02/09/15 10:11, Christian Bruel wrote: >> Hi, >> >> This patch uses TARGET_OPTION_RESTORE and SAVE to switch the attribute >> target dependent params between functions. >> >> This is more efficient than arm_option_params_internal, and prepares the >> ground for the other machine target attributes. > > To be honest, I'm reluctant to take this approach. > The design I'd like to see is for us to figure out the minimal set > of TargetSave variables (and options in arm.opt that require the tag Save) > and use them to 'package' the backend state whenever needed, with the help > of TARGET_OPTION_SAVE when appropriate. > > Then during TARGET_OPTION_RESTORE we would call arm_option_override_internal > and arm_option_params_internal to restore all the different backend > variables that we need. > > In my opinion that is the approach that would give the most maintainability. > In any case, I see arm_option_params_internal is not particularly complicated > at the moment, so I don't think your patch would show any noticeable speed improvement > (unless you can share data to demonstrate that :)) > yes, both approaches are equally not-complex. Calling arm_option_params_internal instead of saving/restoring variables costs a couple of if-then-elses, I don't think the gain can be measurable with the hooks indeed. I wrongly thought the hook could justify the saving, but note that maintainability is not in sake because arm_option_override_internal is still the central point for the initial settings. > Maybe we can make the roadmap for this more concrete if you post a description of > what parameters you'd like to save and restore to achieve this goal and we can discuss > that. Some that come to mind would be: CPU we're tuning for, architecture, FPU, and > basically any option in arm.opt which we want to allow to vary on a per-function basis > (i.e. no ABI-changing options). For now I only expect to support -mfpu. And for this one I only need to save arm_fpu_index, since the FPU params are indexed by it. So OK lets keep calling arm_option_params_internal without new cl_target_option fields. Potentially all the other -moptions that don't create link-time conflicts will be candidate, looking at arm.opt there are not so many. Tuning options like -mrestrict-it or other neon (mneon-for-64bits) would be more important and certainly come first in the list, but now that the infra is there that should be quite fast. Best Regards Christian > > Hope this helps, > > Kyrill > > P.S. > I recently implemented this functionality in the aarch64 backend. > Maybe the implementation of these hooks over there could be of some help > (though I appreciate the arm backend is more complicated with more legacy and option variations) > >> No regressions. OK for trunk ? >> >> many thanks >> >> Christian >> > > Index: gcc/config/arm/arm.c > =================================================================== > --- gcc/config/arm/arm.c (revision 227366) > +++ gcc/config/arm/arm.c (working copy) > @@ -245,9 +245,14 @@ > static void arm_expand_builtin_va_start (tree, rtx); > static tree arm_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *); > static void arm_option_override (void); > +static void arm_option_print (FILE *, int, struct cl_target_option *); > static void arm_set_current_function (tree); > static bool arm_can_inline_p (tree, tree); > static bool arm_valid_target_attribute_p (tree, tree, tree, int); > +static void arm_function_specific_save (struct cl_target_option *, > + struct gcc_options *); > +static void arm_function_specific_restore (struct gcc_options *, > + struct cl_target_option *); > > > I'd rather call them arm_option_save and arm_option_restore. > That way it's easy to deduce that they implement TARGET_OPTION_{SAVE,RESTORE}. >