Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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>>> >>> 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 >>> >>> 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
Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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>>> >>> 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 >>> >>> 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
Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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>>> >>> 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 >>> >>> 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
Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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'hommePR target/85261 * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand into register. *** gcc/testsuite/ChangeLog *** 2018-04-06 Thomas Preud'homme 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
Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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'hommePR target/85261 * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand into register. *** gcc/testsuite/ChangeLog *** 2018-04-06 Thomas Preud'homme 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
Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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> > 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 > > 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
[PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin
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'hommePR target/85261 * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand into register. *** gcc/testsuite/ChangeLog *** 2018-04-06 Thomas Preud'homme 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 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 } } */