| Message ID | CAKdteOYRrE7Jyx8W0ugMHHt2-RT_k8bREuSafcxhWfAzhR33FA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping? On 27 November 2015 at 14:00, Christophe Lyon <christophe.lyon@linaro.org> 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. > I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68620 for this. > > 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? > > Christophe > > > 2015-11-27 Christophe Lyon <christophe.lyon@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.
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 handling this! Kyrill > Christophe > > > 2015-11-27 Christophe Lyon <christophe.lyon@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.
On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkachov@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. Christophe. > Thanks for handling this! > Kyrill > > >> Christophe >> >> >> 2015-11-27 Christophe Lyon <christophe.lyon@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. > >
Hi Christophe, On 08/12/15 11:18, Christophe Lyon wrote: > On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkachov@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 I suspect under your new predicates they'd need to be changed to check for arm_fp_ok rather than arm_vfp_ok. 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? 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? Sorry for not catching these sooner. Kyrill > Christophe. > >> Thanks for handling this! >> Kyrill >> >> >>> Christophe >>> >>> >>> 2015-11-27 Christophe Lyon <christophe.lyon@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. >>
On 10 December 2015 at 13:30, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > Hi Christophe, > > > On 08/12/15 11:18, Christophe Lyon wrote: >> >> On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkachov@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.lyon@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. >>> >>> >
On 10/12/15 13:04, Christophe Lyon wrote: > On 10 December 2015 at 13:30, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> Hi Christophe, >> >> >> On 08/12/15 11:18, Christophe Lyon wrote: >>> On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkachov@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. My gcc is arm-none-eabi configured with --with-float=hard --with-cpu=cortex-a15 --with-fpu=neon-vfpv4 --with-float=hard >> 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. > In the test log where it checks the effective target it fails due to: arm_vfp_ok27268.c:6:4: error: #error __ARM_NEON_FP defined it's compiling the check with -mthumb -march=armv8-a -mfpu=crypto-neon-fp-armv8 -mfloat-abi=hard -o arm_vfp_ok27268.o arm_vfp_ok27268.c >> 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? That's the one that was brought to my attention, but I think the issue here is just the tests that check for arm_vfp_ok rather than the new arm_fp_ok and set -mfpu=vfp explicitly. They appear unsupported if testing with an explicit neon option in mfpu, I think. >> 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. So, I think the action plan here is to audit the tests that need to be changed to arm_fp_ok and add the documentation for the new predicate checks. Thanks, Kyrill > Christophe. > >> Kyrill >> >> >>> Christophe. >>> >>>> Thanks for handling this! >>>> Kyrill >>>> >>>> >>>>> Christophe >>>>> >>>>> >>>>> 2015-11-27 Christophe Lyon <christophe.lyon@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. >>>>
On 10 December 2015 at 14:14, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > > On 10/12/15 13:04, Christophe Lyon wrote: >> >> On 10 December 2015 at 13:30, Kyrill Tkachov <kyrylo.tkachov@arm.com> >> wrote: >>> >>> Hi Christophe, >>> >>> >>> On 08/12/15 11:18, Christophe Lyon wrote: >>>> >>>> On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkachov@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. > > > My gcc is arm-none-eabi configured with > --with-float=hard --with-cpu=cortex-a15 --with-fpu=neon-vfpv4 > --with-float=hard > >>> 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. >> > > In the test log where it checks the effective target it fails due to: > arm_vfp_ok27268.c:6:4: error: #error __ARM_NEON_FP defined > it's compiling the check with > -mthumb -march=armv8-a -mfpu=crypto-neon-fp-armv8 -mfloat-abi=hard -o > arm_vfp_ok27268.o arm_vfp_ok27268.c > >>> 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? > > > That's the one that was brought to my attention, but I think the issue here > is just the > tests that check for arm_vfp_ok rather than the new arm_fp_ok and set > -mfpu=vfp explicitly. > They appear unsupported if testing with an explicit neon option in mfpu, I > think. > >>> 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. > > > So, I think the action plan here is to audit the tests that need to be > changed > to arm_fp_ok and add the documentation for the new predicate checks. > I came up with a new version where I now use only the new arm_fp_ok, so it might be easier just to update arm_vfp_ok and not introduce arm_fp_ok. However, I'm still not happy with this version because it has problems with a few tests that use target attributes such as attr-crypto.c, which starts with: #pragma GCC target ("fpu=crypto-neon-fp-armv8") This pragma fails if the compiler+options have set an incompatible fpu before processing the pragma. So for instance, if gcc has been configured --with-fpu=neon, compiling the test with no fpu option fails (neon is not compatible with crypto-neon-fp-armv8 because the latter has fp16) Now, if we use effective_target arm_vfp_ok + dg_option to force -mfpu=vfp, we end up compiling with -mfpu=vfp and the pragma passes. But if, in addition, we force the testflags to set -mfpu=crypto-neon-fp-armv8 as you do, the effective_target arm_vfp_ok test now fails because it is compiled with -mfpu=vfp -mfpu=crypto-neon-fp-armv8, which defines __ARM_NEON_FP In short, the problem is how to make sure that the fpu setting is always compatible with the pragma, whatever the gcc configuration and multilib options used. Thanks, Christophe. > Thanks, > Kyrill > > >> Christophe. >> >>> Kyrill >>> >>> >>>> Christophe. >>>> >>>>> Thanks for handling this! >>>>> Kyrill >>>>> >>>>> >>>>>> Christophe >>>>>> >>>>>> >>>>>> 2015-11-27 Christophe Lyon <christophe.lyon@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. >>>>> >>>>> >
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 254c4e3..886ad66 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -2664,17 +2664,34 @@ proc check_effective_target_arm_vect_no_misalign { } { # Return 1 if this is an ARM target supporting -mfpu=vfp -# -mfloat-abi=softfp. Some multilibs may be incompatible with these -# options. +# -mfloat-abi=softfp or equivalent options. Some multilibs may be +# incompatible with these options. Also set et_arm_vfp_flags to the +# best options to add. -proc check_effective_target_arm_vfp_ok { } { +proc check_effective_target_arm_vfp_ok_nocache { } { + global et_arm_vfp_flags + set et_arm_vfp_flags "" if { [check_effective_target_arm32] } { - return [check_no_compiler_messages arm_vfp_ok object { - int dummy; - } "-mfpu=vfp -mfloat-abi=softfp"] - } else { - return 0 + foreach flags {"-mfpu=vfp" "-mfpu=vfp -mfloat-abi=softfp" "-mfpu=vfp -mfloat-abi=hard"} { + if { [check_no_compiler_messages_nocache arm_vfp_ok object { + #ifndef __ARM_FP + #error __ARM_FP not defined + #endif + #ifdef __ARM_NEON_FP + #error __ARM_NEON_FP defined + #endif + } "$flags" ] } { + set et_arm_vfp_flags $flags + return 1 + } + } } + return 0 +} + +proc check_effective_target_arm_vfp_ok { } { + return [check_cached_effective_target arm_vfp_ok \ + check_effective_target_arm_vfp_ok_nocache] } # Return 1 if this is an ARM target supporting -mfpu=vfp3 @@ -2721,6 +2738,47 @@ proc check_effective_target_arm_hard_vfp_ok { } { } } +# Return 1 if this is an ARM target defining __ARM_FP. We may need +# -mfloat-abi=softfp or equivalent options. Some multilibs may be +# incompatible with these options. Also set et_arm_fp_flags to the +# best options to add. + +proc check_effective_target_arm_fp_ok_nocache { } { + global et_arm_fp_flags + set et_arm_fp_flags "" + if { [check_effective_target_arm32] } { + foreach flags {"" "-mfloat-abi=softfp" "-mfloat-abi=hard"} { + if { [check_no_compiler_messages_nocache arm_fp_ok object { + #ifndef __ARM_FP + #error __ARM_FP not defined + #endif + } "$flags"] } { + set et_arm_fp_flags $flags + return 1 + } + } + } + + return 0 +} + +proc check_effective_target_arm_fp_ok { } { + return [check_cached_effective_target arm_fp_ok \ + check_effective_target_arm_fp_ok_nocache] +} + +# Add the options needed to define __ARM_FP. We need either +# -mfloat-abi=softfp or -mfloat-abi=hard, but if one is already +# specified by the multilib, use it. + +proc add_options_for_arm_fp { flags } { + if { ! [check_effective_target_arm_fp_ok] } { + return "$flags" + } + global et_arm_fp_flags + return "$flags $et_arm_fp_flags" +} + # Return 1 if this is an ARM target that supports DSP multiply with # current multilib flags. @@ -2753,7 +2811,7 @@ proc check_effective_target_arm_unaligned { } { proc check_effective_target_arm_crypto_ok_nocache { } { global et_arm_crypto_flags set et_arm_crypto_flags "" - if { [check_effective_target_arm32] } { + if { [check_effective_target_arm_v8_neon_ok] } { foreach flags {"" "-mfloat-abi=softfp" "-mfpu=crypto-neon-fp-armv8" "-mfpu=crypto-neon-fp-armv8 -mfloat-abi=softfp"} { if { [check_no_compiler_messages_nocache arm_crypto_ok object { #include "arm_neon.h" @@ -2779,6 +2837,54 @@ proc check_effective_target_arm_crypto_ok { } { check_effective_target_arm_crypto_ok_nocache] } +# Return 1 if this is an ARM target supporting pragma target +# -mfpu=crypto-neon-fp-armv8 -mfloat-abi=softfp or equivalent options. +# Some multilibs may be incompatible with these options. Also set +# et_arm_crypto_pragma_flags to the best options to add. + +proc check_effective_target_arm_crypto_pragma_ok_nocache { } { + global et_arm_crypto_pragma_flags + set et_arm_crypto_pragma_flags "" + if { [check_effective_target_arm_v8_neon_ok] } { + foreach flags {"" "-mfloat-abi=softfp" "-mfpu=crypto-neon-fp-armv8" "-mfpu=crypto-neon-fp-armv8 -mfloat-abi=softfp"} { + if { [check_no_compiler_messages_nocache arm_crypto_pragmaok object { + #pragma GCC target ("fpu=crypto-neon-fp-armv8") + #include "arm_neon.h" + uint8x16_t + foo (uint8x16_t a, uint8x16_t b) + { + return vaeseq_u8 (a, b); + } + } "[add_options_for_arm_v8_neon ""] $flags"] } { + set et_arm_crypto_pragma_flags "[add_options_for_arm_v8_neon ""] $flags" + return 1 + } + } + } + + return 0 +} + +# Return 1 if this is an ARM target supporting pragma target +# -mfpu=crypto-neon-fp-armv8. + +proc check_effective_target_arm_crypto_pragma_ok { } { + return [check_cached_effective_target arm_crypto_pragma_ok \ + check_effective_target_arm_crypto_pragma_ok_nocache] +} + +# Add the options needed for VFP. We need either -mfloat-abi=softfp +# or -mfloat-abi=hard, but if one is already specified by the +# multilib, use it. + +proc add_options_for_arm_vfp { flags } { + if { ! [check_effective_target_arm_vfp_ok] } { + return "$flags" + } + global et_arm_vfp_flags + return "$flags $et_arm_vfp_flags" +} + # Add options for crypto extensions. proc add_options_for_arm_crypto { flags } { if { ! [check_effective_target_arm_crypto_ok] } { @@ -2897,8 +3003,8 @@ proc check_effective_target_arm_crc_ok { } { # Return 1 if this is an ARM target supporting -mfpu=neon-fp16 # -mfloat-abi=softfp or equivalent options. Some multilibs may be -# incompatible with these options. Also set et_arm_neon_flags to the -# best options to add. +# incompatible with these options. Also set et_arm_neon_fp16_flags to +# the best options to add. proc check_effective_target_arm_neon_fp16_ok_nocache { } { global et_arm_neon_fp16_flags diff --git a/gcc/testsuite/gcc.target/arm/attr-crypto.c b/gcc/testsuite/gcc.target/arm/attr-crypto.c index 1db5984..b703fbc 100644 --- a/gcc/testsuite/gcc.target/arm/attr-crypto.c +++ b/gcc/testsuite/gcc.target/arm/attr-crypto.c @@ -1,6 +1,10 @@ /* { dg-do compile } */ -/* { dg-require-effective-target arm_crypto_ok } */ -/* { dg-options "-O2 -mfloat-abi=softfp" } */ +/* Make sure we can force fpu=vfp before switching using the + pragma. */ +/* { dg-require-effective-target arm_vfp_ok } */ +/* { dg-require-effective-target arm_crypto_pragma_ok } */ +/* { dg-options "-O2 -march=armv8-a" } */ +/* { dg-add-options arm_vfp } */ #pragma GCC target ("fpu=crypto-neon-fp-armv8") diff --git a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail.c b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail.c index 6ac32fc..05dc579 100644 --- a/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail.c +++ b/gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail.c @@ -1,7 +1,9 @@ /* Check that calling a neon builtin from a function compiled with vfp fails. */ /* { dg-do compile } */ +/* { dg-require-effective-target arm_fp_ok } */ /* { dg-require-effective-target arm_neon_ok } */ -/* { dg-options "-O2 -mfloat-abi=softfp" } */ +/* { dg-options "-O2" } */ +/* { dg-add-options arm_fp } */ #include <arm_neon.h> @@ -12,6 +14,5 @@ foo (uint8x16_t *p) *p = vmovq_n_u8 (3); /* { dg-message "called from here" } */ } -/* { dg-error "inlining failed in call to always_inline" "" { target *-*-* } 0 } - */ +/* { dg-error "inlining failed in call to always_inline" "" { target *-*-* } 0 } */ diff --git a/gcc/testsuite/gcc.target/arm/attr-neon-fp16.c b/gcc/testsuite/gcc.target/arm/attr-neon-fp16.c index 3cf8918..984992f 100644 --- a/gcc/testsuite/gcc.target/arm/attr-neon-fp16.c +++ b/gcc/testsuite/gcc.target/arm/attr-neon-fp16.c @@ -1,6 +1,7 @@ /* { dg-do compile } */ -/* { dg-require-effective-target arm_neon_ok } */ -/* { dg-options "-mfp16-format=ieee -mfloat-abi=softfp" } */ +/* { dg-require-effective-target arm_fp_ok } */ +/* { dg-options "-mfp16-format=ieee" } */ +/* { dg-add-options arm_fp } */ #include "arm_neon.h" diff --git a/gcc/testsuite/gcc.target/arm/attr-neon2.c b/gcc/testsuite/gcc.target/arm/attr-neon2.c index 819fad4..79133e5 100644 --- a/gcc/testsuite/gcc.target/arm/attr-neon2.c +++ b/gcc/testsuite/gcc.target/arm/attr-neon2.c @@ -1,6 +1,8 @@ /* { dg-do compile } */ /* { dg-require-effective-target arm_neon_ok } */ -/* { dg-options "-O2 -mfloat-abi=softfp -mfpu=vfp" } */ +/* { dg-require-effective-target arm_vfp_ok } */ +/* { dg-options "-O2" } */ +/* { dg-add-options arm_vfp } */ #pragma GCC target ("fpu=neon") #include <arm_neon.h> diff --git a/gcc/testsuite/gcc.target/arm/attr-neon3.c b/gcc/testsuite/gcc.target/arm/attr-neon3.c index 30a1479..0d31fb5 100644 --- a/gcc/testsuite/gcc.target/arm/attr-neon3.c +++ b/gcc/testsuite/gcc.target/arm/attr-neon3.c @@ -1,6 +1,8 @@ /* { dg-do compile } */ /* { dg-require-effective-target arm_crypto_ok } */ -/* { dg-options "-O2 -mfloat-abi=softfp -mfpu=vfp" } */ +/* { dg-require-effective-target arm_vfp_ok } */ +/* { dg-options "-O2" } */ +/* { dg-add-options arm_vfp } */ #include <arm_neon.h>
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? Christophe 2015-11-27 Christophe Lyon <christophe.lyon@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.