| Message ID | d78ea5ca-754c-6291-6acc-4dbc3b8a1359@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin | expand |
On 06/04/2018 16:54, Thomas Preudhomme wrote: > Instruction pattern for setting the FPSCR expects the input value to be > in a register. However, __builtin_arm_set_fpscr expander does not ensure > that this is the case and as a result GCC ICEs when the builtin is > called with a constant literal. > > This commit fixes the builtin to force the input value into a register. > It also remove the unneeded volatile in the existing fpscr test and > fixes the function prototype. > > ChangeLog entries are as follows: > > *** gcc/ChangeLog *** > > 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> > > PR target/85261 > * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand > into register. > > *** gcc/testsuite/ChangeLog *** > > 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> > > PR target/85261 > * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with > literal value. Expect 2 MCR instruction. Fix function prototype. > Remove volatile keyword. > > Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows > no regression. > > Is this ok for stage4? > > Best regards, > > Thomas > (sorry about the duplicate for those who get it) LGTM, though in this case I would prefer a bootstrap and regression run as this is automatically exercised most with gcc.dg/atomic_*.c and you really need this tested on linux than just bare-metal as I'm not sure how this gets tested on arm-none-eabi. What about earlier branches, have you looked ? This is a silly target bug and fixes should go back to older branches in this particular case after baking this on trunk for some time. regards Ramana
On 06/04/18 17:08, Ramana Radhakrishnan wrote: > On 06/04/2018 16:54, Thomas Preudhomme wrote: >> Instruction pattern for setting the FPSCR expects the input value to be >> in a register. However, __builtin_arm_set_fpscr expander does not ensure >> that this is the case and as a result GCC ICEs when the builtin is >> called with a constant literal. >> >> This commit fixes the builtin to force the input value into a register. >> It also remove the unneeded volatile in the existing fpscr test and >> fixes the function prototype. >> >> ChangeLog entries are as follows: >> >> *** gcc/ChangeLog *** >> >> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >> >> PR target/85261 >> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand >> into register. >> >> *** gcc/testsuite/ChangeLog *** >> >> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >> >> PR target/85261 >> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with >> literal value. Expect 2 MCR instruction. Fix function prototype. >> Remove volatile keyword. >> >> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows >> no regression. >> >> Is this ok for stage4? >> >> Best regards, >> >> Thomas >> > > (sorry about the duplicate for those who get it) > > > LGTM, though in this case I would prefer a bootstrap and regression run > as this is automatically exercised most with gcc.dg/atomic_*.c and you > really need this tested on linux than just bare-metal as I'm not sure > how this gets tested on arm-none-eabi. Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap right away. > > What about earlier branches, have you looked ? This is a silly target > bug and fixes should go back to older branches in this particular case > after baking this on trunk for some time. GCC 6 and 7 are affected as well and a backport will be done once it has baked long enough of course. Best regards, Thomas
Hi Ramana, On 06/04/18 17:17, Thomas Preudhomme wrote: > > > On 06/04/18 17:08, Ramana Radhakrishnan wrote: >> On 06/04/2018 16:54, Thomas Preudhomme wrote: >>> Instruction pattern for setting the FPSCR expects the input value to be >>> in a register. However, __builtin_arm_set_fpscr expander does not ensure >>> that this is the case and as a result GCC ICEs when the builtin is >>> called with a constant literal. >>> >>> This commit fixes the builtin to force the input value into a register. >>> It also remove the unneeded volatile in the existing fpscr test and >>> fixes the function prototype. >>> >>> ChangeLog entries are as follows: >>> >>> *** gcc/ChangeLog *** >>> >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >>> >>> PR target/85261 >>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand >>> into register. >>> >>> *** gcc/testsuite/ChangeLog *** >>> >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >>> >>> PR target/85261 >>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with >>> literal value. Expect 2 MCR instruction. Fix function prototype. >>> Remove volatile keyword. >>> >>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows >>> no regression. >>> >>> Is this ok for stage4? >>> >>> Best regards, >>> >>> Thomas >>> >> >> (sorry about the duplicate for those who get it) >> >> >> LGTM, though in this case I would prefer a bootstrap and regression run >> as this is automatically exercised most with gcc.dg/atomic_*.c and you >> really need this tested on linux than just bare-metal as I'm not sure >> how this gets tested on arm-none-eabi. > > Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap > right away. Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4 --with-float=hard --enable-languages=c,c++,fortran --with-system-zlib --enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any regression either. Ok to commit? > >> >> What about earlier branches, have you looked ? This is a silly target >> bug and fixes should go back to older branches in this particular case >> after baking this on trunk for some time. > > GCC 6 and 7 are affected as well and a backport will be done once it has baked > long enough of course. Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that is finished. Best regards, Thomas
Hi Thomas, On 09/04/18 15:29, Thomas Preudhomme wrote: > Hi Ramana, > > On 06/04/18 17:17, Thomas Preudhomme wrote: > > > > > > On 06/04/18 17:08, Ramana Radhakrishnan wrote: > >> On 06/04/2018 16:54, Thomas Preudhomme wrote: > >>> Instruction pattern for setting the FPSCR expects the input value to be > >>> in a register. However, __builtin_arm_set_fpscr expander does not ensure > >>> that this is the case and as a result GCC ICEs when the builtin is > >>> called with a constant literal. > >>> > >>> This commit fixes the builtin to force the input value into a register. > >>> It also remove the unneeded volatile in the existing fpscr test and > >>> fixes the function prototype. > >>> > >>> ChangeLog entries are as follows: > >>> > >>> *** gcc/ChangeLog *** > >>> > >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> > >>> > >>> PR target/85261 > >>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand > >>> into register. > >>> > >>> *** gcc/testsuite/ChangeLog *** > >>> > >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> > >>> > >>> PR target/85261 > >>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with > >>> literal value. Expect 2 MCR instruction. Fix function prototype. > >>> Remove volatile keyword. > >>> > >>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows > >>> no regression. > >>> > >>> Is this ok for stage4? > >>> > >>> Best regards, > >>> > >>> Thomas > >>> > >> > >> (sorry about the duplicate for those who get it) > >> > >> > >> LGTM, though in this case I would prefer a bootstrap and regression run > >> as this is automatically exercised most with gcc.dg/atomic_*.c and you > >> really need this tested on linux than just bare-metal as I'm not sure > >> how this gets tested on arm-none-eabi. > > > > Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap > > right away. > > Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4 > --with-float=hard --enable-languages=c,c++,fortran --with-system-zlib > --enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any > regression either. > > Ok to commit? > Thanks for doing this. This is ok for trunk. > > > >> > >> What about earlier branches, have you looked ? This is a silly target > >> bug and fixes should go back to older branches in this particular case > >> after baking this on trunk for some time. > > > > GCC 6 and 7 are affected as well and a backport will be done once it has baked > > long enough of course. > > Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that > is finished. Thanks, Kyrill > > Best regards, > > Thomas
Hi Kyrill, On 11/04/18 10:02, Kyrill Tkachov wrote: > Hi Thomas, > > On 09/04/18 15:29, Thomas Preudhomme wrote: >> Hi Ramana, >> >> On 06/04/18 17:17, Thomas Preudhomme wrote: >> > >> > >> > On 06/04/18 17:08, Ramana Radhakrishnan wrote: >> >> On 06/04/2018 16:54, Thomas Preudhomme wrote: >> >>> Instruction pattern for setting the FPSCR expects the input value to be >> >>> in a register. However, __builtin_arm_set_fpscr expander does not ensure >> >>> that this is the case and as a result GCC ICEs when the builtin is >> >>> called with a constant literal. >> >>> >> >>> This commit fixes the builtin to force the input value into a register. >> >>> It also remove the unneeded volatile in the existing fpscr test and >> >>> fixes the function prototype. >> >>> >> >>> ChangeLog entries are as follows: >> >>> >> >>> *** gcc/ChangeLog *** >> >>> >> >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >> >>> >> >>> PR target/85261 >> >>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand >> >>> into register. >> >>> >> >>> *** gcc/testsuite/ChangeLog *** >> >>> >> >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >> >>> >> >>> PR target/85261 >> >>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with >> >>> literal value. Expect 2 MCR instruction. Fix function prototype. >> >>> Remove volatile keyword. >> >>> >> >>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows >> >>> no regression. >> >>> >> >>> Is this ok for stage4? >> >>> >> >>> Best regards, >> >>> >> >>> Thomas >> >>> >> >> >> >> (sorry about the duplicate for those who get it) >> >> >> >> >> >> LGTM, though in this case I would prefer a bootstrap and regression run >> >> as this is automatically exercised most with gcc.dg/atomic_*.c and you >> >> really need this tested on linux than just bare-metal as I'm not sure >> >> how this gets tested on arm-none-eabi. >> > >> > Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap >> > right away. >> >> Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4 >> --with-float=hard --enable-languages=c,c++,fortran --with-system-zlib >> --enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any >> regression either. >> >> Ok to commit? >> > > Thanks for doing this. > This is ok for trunk. > >> > >> >> >> >> What about earlier branches, have you looked ? This is a silly target >> >> bug and fixes should go back to older branches in this particular case >> >> after baking this on trunk for some time. >> > >> > GCC 6 and 7 are affected as well and a backport will be done once it has baked >> > long enough of course. >> >> Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that >> is finished. Backports show no regression on a bootstrapped arm-none-linux-gnueabihf GCC 6 & 7. Ok to commit those? Best regards, Thomas
Hi Thomas, On 18/04/18 12:31, Thomas Preudhomme wrote: > Hi Kyrill, > > On 11/04/18 10:02, Kyrill Tkachov wrote: >> Hi Thomas, >> >> On 09/04/18 15:29, Thomas Preudhomme wrote: >>> Hi Ramana, >>> >>> On 06/04/18 17:17, Thomas Preudhomme wrote: >>> > >>> > >>> > On 06/04/18 17:08, Ramana Radhakrishnan wrote: >>> >> On 06/04/2018 16:54, Thomas Preudhomme wrote: >>> >>> Instruction pattern for setting the FPSCR expects the input value to be >>> >>> in a register. However, __builtin_arm_set_fpscr expander does not ensure >>> >>> that this is the case and as a result GCC ICEs when the builtin is >>> >>> called with a constant literal. >>> >>> >>> >>> This commit fixes the builtin to force the input value into a register. >>> >>> It also remove the unneeded volatile in the existing fpscr test and >>> >>> fixes the function prototype. >>> >>> >>> >>> ChangeLog entries are as follows: >>> >>> >>> >>> *** gcc/ChangeLog *** >>> >>> >>> >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >>> >>> >>> >>> PR target/85261 >>> >>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand >>> >>> into register. >>> >>> >>> >>> *** gcc/testsuite/ChangeLog *** >>> >>> >>> >>> 2018-04-06 Thomas Preud'homme <thomas.preudhomme@arm.com> >>> >>> >>> >>> PR target/85261 >>> >>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with >>> >>> literal value. Expect 2 MCR instruction. Fix function prototype. >>> >>> Remove volatile keyword. >>> >>> >>> >>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows >>> >>> no regression. >>> >>> >>> >>> Is this ok for stage4? >>> >>> >>> >>> Best regards, >>> >>> >>> >>> Thomas >>> >>> >>> >> >>> >> (sorry about the duplicate for those who get it) >>> >> >>> >> >>> >> LGTM, though in this case I would prefer a bootstrap and regression run >>> >> as this is automatically exercised most with gcc.dg/atomic_*.c and you >>> >> really need this tested on linux than just bare-metal as I'm not sure >>> >> how this gets tested on arm-none-eabi. >>> > >>> > Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap >>> > right away. >>> >>> Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4 >>> --with-float=hard --enable-languages=c,c++,fortran --with-system-zlib >>> --enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any >>> regression either. >>> >>> Ok to commit? >>> >> >> Thanks for doing this. >> This is ok for trunk. >> >>> > >>> >> >>> >> What about earlier branches, have you looked ? This is a silly target >>> >> bug and fixes should go back to older branches in this particular case >>> >> after baking this on trunk for some time. >>> > >>> > GCC 6 and 7 are affected as well and a backport will be done once it has baked >>> > long enough of course. >>> >>> Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that >>> is finished. > > Backports show no regression on a bootstrapped arm-none-linux-gnueabihf GCC 6 & 7. Ok to commit those? > Yes, thanks. Kyrill > Best regards, > > Thomas
diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c index 8940d1f6311bccf86664ab2eaa938735eec595f6..e100d933a77c5de4a13cb961d1bff40f57f2ea80 100644 --- a/gcc/config/arm/arm-builtins.c +++ b/gcc/config/arm/arm-builtins.c @@ -2592,7 +2592,7 @@ arm_expand_builtin (tree exp, icode = CODE_FOR_set_fpscr; arg0 = CALL_EXPR_ARG (exp, 0); op0 = expand_normal (arg0); - pat = GEN_FCN (icode) (op0); + pat = GEN_FCN (icode) (force_reg (SImode, op0)); } emit_insn (pat); return target; diff --git a/gcc/testsuite/gcc.target/arm/fpscr.c b/gcc/testsuite/gcc.target/arm/fpscr.c index 7b4d71d72d8964f6da0d0604bf59aeb4a895df43..4c3eaf7fcf75ad8582071ecb110fd1e4976a3b24 100644 --- a/gcc/testsuite/gcc.target/arm/fpscr.c +++ b/gcc/testsuite/gcc.target/arm/fpscr.c @@ -6,11 +6,14 @@ /* { dg-add-options arm_fp } */ void -test_fpscr () +test_fpscr (void) { - volatile unsigned int status = __builtin_arm_get_fpscr (); + unsigned status; + + __builtin_arm_set_fpscr (0); + 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" } } */ +/* { dg-final { scan-assembler-times "mcr\tp10, 7, r\[0-9\]+, cr1, cr0, 0" 2 } } */