| Message ID | 9982d9c9-ee19-c423-1e47-d0993987d369@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Thomas, On 23/06/17 16:48, Thomas Preudhomme wrote: > Hi Kyrill, > > On 10/04/17 15:01, Kyrill Tkachov wrote: >> Hi Prakhar, >> Sorry for the delay, >> >> On 22/03/17 10:46, Prakhar Bahuguna wrote: >>> The GCC documentation in section 6.60.8 ARM Floating Point Status and Control >>> Intrinsics states that the FPSCR register can be read and written to using the >>> intrinsics __builtin_arm_get_fpscr and __builtin_arm_set_fpscr. However, these >>> are misnamed within GCC itself and these intrinsic names are not recognised. >>> This patch corrects the intrinsic names to match the documentation, and adds >>> tests to verify these intrinsics generate the correct instructions. >>> >>> Testing done: Ran regression tests on arm-none-eabi for Cortex-M4. >>> >>> 2017-03-09 Prakhar Bahuguna <prakhar.bahuguna@arm.com> >>> >>> gcc/ChangeLog: >>> >>> * gcc/config/arm/arm-builtins.c (arm_init_builtins): Rename >>> __builtin_arm_ldfscr to __builtin_arm_get_fpscr, and rename >>> __builtin_arm_stfscr to __builtin_arm_set_fpscr. >>> * gcc/testsuite/gcc.target/arm/fpscr.c: New file. >>> >>> Okay for stage 1? >> >> I see that the mistake was in not addressing one of the review comments in: >> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01832.html >> properly in the patch that added these functions :( >> >> This is ok for stage 1 if a bootstrap and test on arm-none-linux-gnueabihf works >> fine >> I don't think we want to maintain the __builtin_arm_[ld,st]fscr names for >> backwards compatibility >> as they were not documented and are __builtin_arm* functions that we don't >> guarantee to maintain. > > How about a backport to GCC 5, 6 & 7? The patch applied cleanly on each of these versions and the testsuite didn't show any regression for any of the backport when run for Cortex-M7. Yes, thanks. These were always documented "correctly". The patch makes sure the implementation matches that documentation. Kyrill > > Patches attached for reference. > > ChangeLog entries: > > *** gcc/ChangeLog *** > > 2017-06-20 Thomas Preud'homme <thomas.preudhomme@arm.com> > > Backport from mainline > 2017-05-04 Prakhar Bahuguna <prakhar.bahuguna@arm.com> > > * gcc/config/arm/arm-builtins.c (arm_init_builtins): Rename > __builtin_arm_ldfscr to __builtin_arm_get_fpscr, and rename > __builtin_arm_stfscr to __builtin_arm_set_fpscr. > > > *** gcc/testsuite/ChangeLog *** > > 2017-06-20 Thomas Preud'homme <thomas.preudhomme@arm.com> > > Backport from mainline > 2017-05-04 Prakhar Bahuguna <prakhar.bahuguna@arm.com> > > gcc/testsuite/ > * gcc.target/arm/fpscr.c: New file. > > > Best regards, > > Thomas
Hi Thomas, On 23 June 2017 at 17:48, Thomas Preudhomme <thomas.preudhomme@foss.arm.com> wrote: > Hi Kyrill, > > > On 10/04/17 15:01, Kyrill Tkachov wrote: >> >> Hi Prakhar, >> Sorry for the delay, >> >> On 22/03/17 10:46, Prakhar Bahuguna wrote: >>> >>> The GCC documentation in section 6.60.8 ARM Floating Point Status and >>> Control >>> Intrinsics states that the FPSCR register can be read and written to >>> using the >>> intrinsics __builtin_arm_get_fpscr and __builtin_arm_set_fpscr. However, >>> these >>> are misnamed within GCC itself and these intrinsic names are not >>> recognised. >>> This patch corrects the intrinsic names to match the documentation, and >>> adds >>> tests to verify these intrinsics generate the correct instructions. >>> >>> Testing done: Ran regression tests on arm-none-eabi for Cortex-M4. >>> >>> 2017-03-09 Prakhar Bahuguna <prakhar.bahuguna@arm.com> >>> >>> gcc/ChangeLog: >>> >>> * gcc/config/arm/arm-builtins.c (arm_init_builtins): Rename >>> __builtin_arm_ldfscr to __builtin_arm_get_fpscr, and rename >>> __builtin_arm_stfscr to __builtin_arm_set_fpscr. >>> * gcc/testsuite/gcc.target/arm/fpscr.c: New file. >>> >>> Okay for stage 1? >> >> >> I see that the mistake was in not addressing one of the review comments >> in: >> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01832.html >> properly in the patch that added these functions :( >> >> This is ok for stage 1 if a bootstrap and test on arm-none-linux-gnueabihf >> works >> fine >> I don't think we want to maintain the __builtin_arm_[ld,st]fscr names for >> backwards compatibility >> as they were not documented and are __builtin_arm* functions that we don't >> guarantee to maintain. > > > How about a backport to GCC 5, 6 & 7? The patch applied cleanly on each of > these versions and the testsuite didn't show any regression for any of the > backport when run for Cortex-M7. > Three's a problem with GCC-5: gcc.target/arm/fpscr.c: unknown effective target keyword `arm_fp_ok' for " dg-require-effective-target 4 arm_fp_ok " Indeed arm_fp_ok effective-target does not exist in the gcc-5 branch. Christophe > Patches attached for reference. > > ChangeLog entries: > > *** gcc/ChangeLog *** > > 2017-06-20 Thomas Preud'homme <thomas.preudhomme@arm.com> > > Backport from mainline > 2017-05-04 Prakhar Bahuguna <prakhar.bahuguna@arm.com> > > * gcc/config/arm/arm-builtins.c (arm_init_builtins): Rename > __builtin_arm_ldfscr to __builtin_arm_get_fpscr, and rename > __builtin_arm_stfscr to __builtin_arm_set_fpscr. > > > *** gcc/testsuite/ChangeLog *** > > 2017-06-20 Thomas Preud'homme <thomas.preudhomme@arm.com> > > Backport from mainline > 2017-05-04 Prakhar Bahuguna <prakhar.bahuguna@arm.com> > > gcc/testsuite/ > * gcc.target/arm/fpscr.c: New file. > > > Best regards, > > Thomas
Hi Christophe, On 23/06/17 20:10, Christophe Lyon wrote: > Hi Thomas, > > On 23 June 2017 at 17:48, Thomas Preudhomme > <thomas.preudhomme@foss.arm.com> wrote: >> Hi Kyrill, >> >> >> On 10/04/17 15:01, Kyrill Tkachov wrote: >>> >>> Hi Prakhar, >>> Sorry for the delay, >>> >>> On 22/03/17 10:46, Prakhar Bahuguna wrote: >>>> >>>> The GCC documentation in section 6.60.8 ARM Floating Point Status and >>>> Control >>>> Intrinsics states that the FPSCR register can be read and written to >>>> using the >>>> intrinsics __builtin_arm_get_fpscr and __builtin_arm_set_fpscr. However, >>>> these >>>> are misnamed within GCC itself and these intrinsic names are not >>>> recognised. >>>> This patch corrects the intrinsic names to match the documentation, and >>>> adds >>>> tests to verify these intrinsics generate the correct instructions. >>>> >>>> Testing done: Ran regression tests on arm-none-eabi for Cortex-M4. >>>> >>>> 2017-03-09 Prakhar Bahuguna <prakhar.bahuguna@arm.com> >>>> >>>> gcc/ChangeLog: >>>> >>>> * gcc/config/arm/arm-builtins.c (arm_init_builtins): Rename >>>> __builtin_arm_ldfscr to __builtin_arm_get_fpscr, and rename >>>> __builtin_arm_stfscr to __builtin_arm_set_fpscr. >>>> * gcc/testsuite/gcc.target/arm/fpscr.c: New file. >>>> >>>> Okay for stage 1? >>> >>> >>> I see that the mistake was in not addressing one of the review comments >>> in: >>> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01832.html >>> properly in the patch that added these functions :( >>> >>> This is ok for stage 1 if a bootstrap and test on arm-none-linux-gnueabihf >>> works >>> fine >>> I don't think we want to maintain the __builtin_arm_[ld,st]fscr names for >>> backwards compatibility >>> as they were not documented and are __builtin_arm* functions that we don't >>> guarantee to maintain. >> >> >> How about a backport to GCC 5, 6 & 7? The patch applied cleanly on each of >> these versions and the testsuite didn't show any regression for any of the >> backport when run for Cortex-M7. >> > > Three's a problem with GCC-5: > gcc.target/arm/fpscr.c: unknown effective target keyword > `arm_fp_ok' for " dg-require-effective-target 4 arm_fp_ok " > > Indeed arm_fp_ok effective-target does not exist in the gcc-5 branch. Oh no. I remember not seeing anything but I can indeed see this with compare_tests from the sum file I save after each testing. Alright, what is done is done, working on a patch now. Best regards, Thomas
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index b23ab131099d8120f02c283edea9d6cac3ed957a..83731befa78a9bf8c672ac52c16f8dd9029757b2 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,12 @@ +2017-06-20 Thomas Preud'homme <thomas.preudhomme@arm.com> + + Backport from mainline + 2017-05-04 Prakhar Bahuguna <prakhar.bahuguna@arm.com> + + * gcc/config/arm/arm-builtins.c (arm_init_builtins): Rename + __builtin_arm_ldfscr to __builtin_arm_get_fpscr, and rename + __builtin_arm_stfscr to __builtin_arm_set_fpscr. + 2017-06-20 Andreas Schwab <schwab@suse.de> PR target/80970 diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c index 8b7eaa8e81c59d0e18e22908d8748f0e01f5a9a2..792b688f66cd666e4fcef568fadc385c5b332be4 100644 --- a/gcc/config/arm/arm-builtins.c +++ b/gcc/config/arm/arm-builtins.c @@ -1893,10 +1893,10 @@ arm_init_builtins (void) = build_function_type_list (unsigned_type_node, NULL); arm_builtin_decls[ARM_BUILTIN_GET_FPSCR] - = add_builtin_function ("__builtin_arm_ldfscr", ftype_get_fpscr, + = add_builtin_function ("__builtin_arm_get_fpscr", ftype_get_fpscr, ARM_BUILTIN_GET_FPSCR, BUILT_IN_MD, NULL, NULL_TREE); arm_builtin_decls[ARM_BUILTIN_SET_FPSCR] - = add_builtin_function ("__builtin_arm_stfscr", ftype_set_fpscr, + = add_builtin_function ("__builtin_arm_set_fpscr", ftype_set_fpscr, ARM_BUILTIN_SET_FPSCR, BUILT_IN_MD, NULL, NULL_TREE); } diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index fe738a0960bf4af1db9bf8c20a4acb515251ad26..82f4a382d4b1df30807b10bfbfcdbb322dbd1722 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2017-06-20 Thomas Preud'homme <thomas.preudhomme@arm.com> + + Backport from mainline + 2017-05-04 Prakhar Bahuguna <prakhar.bahuguna@arm.com> + + * gcc.target/arm/fpscr.c: New file. + 2017-06-19 James Greenhalgh <james.greenhalgh@arm.com> Backport from mainline diff --git a/gcc/testsuite/gcc.target/arm/fpscr.c b/gcc/testsuite/gcc.target/arm/fpscr.c new file mode 100644 index 0000000000000000000000000000000000000000..7b4d71d72d8964f6da0d0604bf59aeb4a895df43 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/fpscr.c @@ -0,0 +1,16 @@ +/* Test the fpscr builtins. */ + +/* { dg-do compile } */ +/* { dg-require-effective-target arm_fp_ok } */ +/* { dg-skip-if "need fp instructions" { *-*-* } { "-mfloat-abi=soft" } { "" } } */ +/* { dg-add-options arm_fp } */ + +void +test_fpscr () +{ + volatile unsigned int status = __builtin_arm_get_fpscr (); + __builtin_arm_set_fpscr (status); +} + +/* { dg-final { scan-assembler "mrc\tp10, 7, r\[0-9\]+, cr1, cr0, 0" } } */ +/* { dg-final { scan-assembler "mcr\tp10, 7, r\[0-9\]+, cr1, cr0, 0" } } */