On 10 December 2015 at 13:30, Kyrill Tkachov <kyrylo.tkac...@arm.com> wrote: > Hi Christophe, > > > On 08/12/15 11:18, Christophe Lyon wrote: >> >> On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkac...@arm.com> >> 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 <christophe.l...@linaro.org> >>>> >>>> * 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. >>> >>> >