From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 64904 invoked by alias); 10 Dec 2015 13:05:09 -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 64852 invoked by uid 89); 10 Dec 2015 13:05:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qg0-f52.google.com Received: from mail-qg0-f52.google.com (HELO mail-qg0-f52.google.com) (209.85.192.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 10 Dec 2015 13:05:04 +0000 Received: by qgea14 with SMTP id a14so141056418qge.0 for ; Thu, 10 Dec 2015 05:05:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=kchIECfIPeM4lWFPOJOUQl93Wv38NIibkasZckUlNt4=; b=EXgbViYuP+XI88gwFwWzNWbxyOpsZXjBKZgnaww3MXbPNOnWFOtaBiunjKLs6rZj/O jXaAODVQVY0TgxSoxeKrvLUmmHyx9Q96WbktbSkB+mlyjrEy7G5qB5eGyEhGXod3+NUQ EXrK3m7LnP0zu0d0QQoMU5h/wGNW177PSuySn49H4gsh9EVt5UV2jqqwkG07+lVxD27+ IzXKfLVIRp8axUmrBpBicnzPnolD9CnfM7yQbgrU0nPLd7lNmWI8hEZVRR5EAeNxYuBE yrKXqE40LXAUV0MMgxe9luw7QJYUVBg01kGi4p2eEGA3rFGDAThNfZiZWW/hOOG/cmxs F38A== X-Gm-Message-State: ALoCoQnSNK+LQNju3nlst1l12mAE1krbZAWX4CGAU55vTKqlLQw5l3jaoVj/niEYW/hrSi+O/KSX427a592dEF+NEd5O/4wiaDZ4+IRnq4e1yzZNE7Scsls= MIME-Version: 1.0 X-Received: by 10.55.207.73 with SMTP id e70mr14831622qkj.77.1449752695987; Thu, 10 Dec 2015 05:04:55 -0800 (PST) Received: by 10.140.82.137 with HTTP; Thu, 10 Dec 2015 05:04:55 -0800 (PST) In-Reply-To: <5669704D.5000000@arm.com> References: <5666B5DA.90104@arm.com> <5669704D.5000000@arm.com> Date: Thu, 10 Dec 2015 13:05:00 -0000 Message-ID: Subject: Re: [testsuite][ARM target attributes] Fix effective_target tests From: Christophe Lyon To: Kyrill Tkachov Cc: "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg01122.txt.bz2 On 10 December 2015 at 13:30, Kyrill Tkachov wrote: > Hi Christophe, > > > On 08/12/15 11:18, Christophe Lyon wrote: >> >> On 8 December 2015 at 11:50, Kyrill Tkachov >> wrote: >>> >>> Hi Christophe, >>> >>> >>> On 27/11/15 13:00, Christophe Lyon wrote: >>>> >>>> Hi, >>>> >>>> After the recent commits from Christian adding target attributes >>>> support for ARM FPU settings, I've noticed that some of the tests >>>> were failing because of incorrect assumptions wrt to the default >>>> cpu/fpu/float-abi of the compiler. >>>> >>>> This patch fixes the problems I've noticed in the following way: >>>> - do not force -mfloat-abi=softfp in dg-options, to avoid conflicts >>>> when gcc is configured --with-float=hard >>>> >>>> - change arm_vfp_ok such that it tries several -mfpu/-mfloat-abi >>>> flags, checks that __ARM_FP is defined and __ARM_NEON_FP is not >>>> defined >>>> >>>> - introduce arm_fp_ok, which is similar but does not enforce fpu setting >>>> >>>> - add a new effective_target: arm_crypto_pragma_ok to check that >>>> setting this fpu via a pragma is actually supported by the current >>>> "multilib". This is different from checking the command-line option >>>> because the pragma might conflict with the command-line options in >>>> use. >>>> >>>> The updates in the testcases are as follows: >>>> - attr-crypto.c, we have to make sure that the defaut fpu does not >>>> conflict with the one forced by pragma. That's why I use the arm_vfp >>>> options/effective_target. This is needed if gcc has been configured >>>> --with-fpu=neon-fp16, as the pragma fpu=crypto-neon-fp-armv8 would >>>> conflict. >>>> >>>> - attr-neon-builtin-fail.c: use arm_fp to force the appropriate >>>> float-abi setting. Enforcing fpu is not needed here. >>>> >>>> - attr-neon-fp16.c: similar, I also removed arm_neon_ok since it was >>>> not necessary to make the test pass in my testing. On second thought, >>>> I'm wondering whether I should leave it and make the test unsupported >>>> in more cases (such as when forcing -march=armv5t, although it does >>>> pass with this patch) >>>> >>>> - attr-neon2.c: use arm_vfp to force the appropriate float-abi >>>> setting. Enforcing mfpu=vfp is needed to avoid conflict with the >>>> pragma target fpu=neon (for instance if the toolchain default is >>>> neon-fp16) >>>> >>>> - attr-neon3.c: similar >>>> >>>> Tested on a variety of configurations, see: >>>> >>>> >>>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/230929-target-attr/report-build-info.html >>>> >>>> Note that the regressions reported fall into 3 categories: >>>> - when forcing march=armv5t: tests are now unsupported because I >>>> modified arm_crypto_ok to require arm_v8_neon_ok instead of arm32. >>>> >>>> - the warning reported by attr-neon-builtin-fail.c moved from line 12 >>>> to 14 and is thus seen as a regression + one improvement >>>> >>>> - finally, attr-neon-fp16.c causes an ICE on armeb compilers, for >>>> which I need to post a bugzilla. >>>> >>>> >>>> TBH, I'm a bit concerned by the complexity of all these multilib-like >>>> conditions. I'm confident that I'm still missing some combinations :-) >>>> >>>> And with new target attributes coming, new architectures etc... all >>>> this logic is likely to become even more complex. >>>> >>>> That being said, OK for trunk? >>> >>> >>> I'd like us to clean up and reorganise the gcc.target/arm testsuite >>> at some point to make it more robust with respect to the tons of multilib >>> options and configurations we can have for arm. It's becoming very >>> frustrating >>> to test feature-specific stuff :( >>> >>> This is ok in the meantime. >>> Sorry for the delay. >>> >> Thanks for the approval, glad to see I'm not the only one willing to see >> improvements in this area :) >> >> Committed as r231403. > > > With this patch we're seeing legitimate PASSes go to NA. > For example: > > gcc.target/arm/vfp-1.c > gcc.target/arm/vfp-ldmdbs.c > and other vfp tests in arm.exp. > This is, for example, for the > variant-mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard > Hmm I'm attempting to cover such a configuration in my matrix of validations: http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/231403/report-build-info.html The difference is that I use similar flags at GCC configure time, while you override them when running the testsuite: --target arm-none-linux-gnueabihf --with-mode=thum --with-cpu=cortex-a57 --with-fpu=crypto-neon-fp-armv8 I this case, I do not see the regressions. > I suspect under your new predicates they'd need to be changed to check for > arm_fp_ok rather than arm_vfp_ok. Probably, yes. > We want to avoid removing any PASSes. > Can you please test some more to make sure we don't remove any legitimate > passes with your patch? Is that the only combination in which you saw less PASSes? > Also, Ramana reminded me offline that any new predicates in > target-supports.exp > need documenting in sourcebuild.txt. > > In light of that, can you please revert your patch and address the issues > above > so that we can be confident we don't lose existing legitimate test coverage? OK. > Sorry for not catching these sooner. No problem, there are so many combinations. I'm not sure how to define a reasonable set. Christophe. > Kyrill > > >> Christophe. >> >>> Thanks for handling this! >>> Kyrill >>> >>> >>>> Christophe >>>> >>>> >>>> 2015-11-27 Christophe Lyon >>>> >>>> * lib/target-supports.exp >>>> (check_effective_target_arm_vfp_ok_nocache): New. >>>> (check_effective_target_arm_vfp_ok): Call the new >>>> check_effective_target_arm_vfp_ok_nocache function. >>>> (check_effective_target_arm_fp_ok_nocache): New. >>>> (check_effective_target_arm_fp_ok): New. >>>> (add_options_for_arm_fp): New. >>>> (check_effective_target_arm_crypto_ok_nocache): Require >>>> target_arm_v8_neon_ok instead of arm32. >>>> (check_effective_target_arm_crypto_pragma_ok_nocache): New. >>>> (check_effective_target_arm_crypto_pragma_ok): New. >>>> (add_options_for_arm_vfp): New. >>>> * gcc.target/arm/attr-crypto.c: Use arm_crypto_pragma_ok effective >>>> target. Do not force -mfloat-abi=softfp, use arm_vfp effective >>>> target instead. >>>> * gcc.target/arm/attr-neon-builtin-fail.c: Do not force >>>> -mfloat-abi=softfp, use arm_fp effective target instead. >>>> * gcc.target/arm/attr-neon-fp16.c: Likewise. Remove arm_neon_ok >>>> dependency. >>>> * gcc.target/arm/attr-neon2.c: Do not force -mfloat-abi=softfp, >>>> use arm_vfp effective target instead. >>>> * gcc.target/arm/attr-neon3.c: Likewise. >>> >>> >