From patchwork Thu Jan 7 12:08:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 564251 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 BD69114029E for ; Thu, 7 Jan 2016 23:08:54 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=M8sCdiSV; 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:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=i2BrdN5hKIKh60UJhiJmXx/L2BQRlVik4cL17w0mbCPdjdi87g/+N +a2gbA/0ItEYfD9u8ilt8xKhmuThANk6kLFc2oKXdBwj7JDTvT4oq349NqcI5lpU 1qxgUMo8ZWjY5lLmMjGvpRGpZhR3WfJdUrbdYIhX2TmP8GEr0rXvek= 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:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=tCQi/GIhpElvoGjFi5GYnjd1nas=; b=M8sCdiSVeUXOLhG27bjoHysA0d7b CLs0qE4ouGowdRR94HNTXAQZjLCeLsD6aj2jI/5cMet7E6BaVNcecQ5lNYtY2Con DrOrHPKrqyj7MUDXOEjoRUxwZxeX9UDdbMC+ewM3UotAn1OuapBhNKCpqDvqreOQ Vq8WuEybUmvXcbU= Received: (qmail 103196 invoked by alias); 7 Jan 2016 12:08:47 -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 103179 invoked by uid 89); 7 Jan 2016 12:08:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.3 required=5.0 tests=AWL, BAYES_50, KAM_ASCII_DIVIDERS, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=PR68674, pr68674, Bruel, bruel 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; Thu, 07 Jan 2016 12:08:44 +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 B18CF3F8; Thu, 7 Jan 2016 04:08:09 -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 006933F246; Thu, 7 Jan 2016 04:08:41 -0800 (PST) Message-ID: <568E5548.6060207@foss.arm.com> Date: Thu, 07 Jan 2016 12:08:40 +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 , "Richard.Earnshaw@arm.com" , "ramana.radhakrishnan@foss.arm.com" , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH, ARM] PR68674 Fix LTO support for neon builtin and error catching (ping) References: <568D1E7B.1090609@st.com> <568D3B10.6080309@foss.arm.com> <568D3F76.2020202@foss.arm.com> <568D4B4D.2010304@st.com> <568D4D28.8010500@foss.arm.com> <568E3E6B.9040909@st.com> In-Reply-To: <568E3E6B.9040909@st.com> Hi Christian, On 07/01/16 10:31, Christian Bruel wrote: > > It seems that changing the arm_cpu_builtins code in arm-c.c to do: > cpp_undef (pfile, "__ARM_NEON_FP"); > if (TARGET_NEON_FP) > builtin_define_with_int_value ("__ARM_NEON_FP", TARGET_NEON_FP); > > instead of: > if (TARGET_NEON_FP) > builtin_define_with_int_value ("__ARM_NEON_FP", TARGET_NEON_FP); > else > cpp_undef (pfile, "__ARM_NEON_FP"); > > fixes this. I guess we should be undefing any macros before redefining them.I > >>> I don't know, If we are going from v7 to v8, the __ARM_NEON_FP value indeed changes. The question is whether we want to hide this to the user ? >> Well, the user explicitly changed the fpu level with the pragma in the testcase, which rightly has the effect of >> changing the value of some predefines. I don't think warning is appropriate here, as long as the predefine >> has the right value in the right pragma scope. >> > note that the issue can be reproduced on the current trunc with the default configure with > > arm-none-eabi-gcc -mfloat-abi=softfp -mfpu=neon-fp-armv8 > -------------------------------------------------- > #pragma GCC target ("fpu=neon") > ------------------------------------------------- > that gives: > > warning: "__ARM_NEON_FP" redefined > : note: this is the location of the previous definition > > So I agree, the warning seems intuitively useless, but I'd just need to mention : > > iso/iec 99 A.3 and 6.10 (Preprocessing directives) #pragma is a preprocessor directive > and iso/iec 99 6.10.3 (Macro Replacement) > > also, you fix is slightly wrong, since the warning should be kept for other explicit #define user redefinitions. And I think we shouldn't use cpp_undef for that. It's better to use NODE_CONDITIONAL flags on the macro identifier, since > this is what it is. > > I'd like to send a patch for this separately, so you can continue reviewing this one independently of the decision > Sure, I see you already filed PR target/69180 for this. Thanks. I'm not sure whether it's valid for the user to redefine ACLE macros like this anyway, but if it's possible to warn them about this and not warn for this implicit redefinition done by the compiler itself, then that would be desirable. > note that for the time being to make the patches independent the current test can be fixed with > > #if __ARM_ARCH==8 > #pragma GCC target ("fpu=neon-fp-armv8") > #else > #pragma GCC target ("fpu=neon") > #endif > We agree that it's a legitimate bug, so I think the error should remain. After all, silencing it by tweaking the testcase doesn't make the underlying bug go away. Depending on how complicated it would be to fix PR 69180 maybe we should apply a fix for that first and then apply this patch? > thanks for catching this with the aarch32 v8 validation. I'll add this option to my validation scripts. > > Cheers > Another nit I spotted in the testcases: Index: gcc/testsuite/gcc.target/arm/lto/pr65837-attr_0.c =================================================================== --- gcc/testsuite/gcc.target/arm/lto/pr65837-attr_0.c (revision 0) +++ gcc/testsuite/gcc.target/arm/lto/pr65837-attr_0.c (working copy) @@ -0,0 +1,17 @@ +/* { dg-lto-do run } */ +/* { dg-require-effective-target arm_neon_ok } */ If you're intending to run the testcases then you should use the arm_neon_hw effective target. On the subject of initialising the builtins unconditionally and the memory/performance impact: Do you think as a first step it would be a good idea to not initialise the builtins if the configuration is not linkable with any possible NEON configuration? For example, -mfloat-abi=soft. We're not allowed to link such modules with any possible module that uses NEON anyway because of the ABI, so at least users of -mfloat-abi=soft wouldn't have to sit through the NEON builtins initialisation. Thanks, Kyrill