From patchwork Fri Jan 22 14:17:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 571698 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 0363114031F for ; Sat, 23 Jan 2016 01:17:39 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=cFTmpc8P; 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=md6McBCYOm+xKDyOXKlzx7A0ZvNd3sdaKuJk7KpVlV6VzwPHWoVSj tOSGiktCLVQBcXTo6qvNLd0GtuVz2cpxgMwMvk0lMpexQrUXeqBCbRJ6zf97wvZV YViYY7XecUOMbbX70rEDgN1ptNog6uPqmr+shCejrXfKajwZp0xwcU= 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=/SY0odYMrZeWKjhzCe10Jnox09A=; b=cFTmpc8PSS2k2L46QJpzhq8FwIU3 sn7f5R9ecdQ7f+F5qZ2GLmCD4kM3iDj8+prNhFG9QJ5YOB3o30x2v32eIcQ8JDbe sehVjlu+U5KAGZJEKwFktgG+ZCLVFzGjsw9wWKfvcbm6RHo0hgZnUZWB/AOHjch8 5FYdLyz/TtwQxLk= Received: (qmail 85711 invoked by alias); 22 Jan 2016 14:17:32 -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 85696 invoked by uid 89); 22 Jan 2016 14:17:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.8 required=5.0 tests=BAYES_50, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=popping, cached, neonmfpu, neon, -mfpu 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; Fri, 22 Jan 2016 14:17:30 +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 1805C3A8; Fri, 22 Jan 2016 06:16:49 -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 960063F246; Fri, 22 Jan 2016 06:17:27 -0800 (PST) Message-ID: <56A239F6.30102@foss.arm.com> Date: Fri, 22 Jan 2016 14:17:26 +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> In-Reply-To: <56A237B4.60407@st.com> 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 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. Thanks, Kyrill 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 } } */