| Message ID | 4db1e5be-24b1-bab7-d351-b00ef5332728@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ARM] Fix wrong code by arm_final_prescan with fp16 move instructions | expand |
Hi Sudi, On 24/11/17 14:57, Sudi Das wrote: > Hi > > For the following test case: > __fp16 > test_select (__fp16 a, __fp16 b, __fp16 c) > { > return (a < b) ? b : c; > } > > when compiled with -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm > -mfloat-abi=hard trunk generates wrong code: > > test_select: > @ args = 0, pretend = 0, frame = 0 > @ frame_needed = 0, uses_anonymous_args = 0 > @ link register save eliminated. > vcvtb.f32.f16 s0, s0 > vcvtb.f32.f16 s15, s1 > vcmpe.f32 s0, s15 > vmrs APSR_nzcv, FPSCR > // <------ No conditional branch! > vmov s1, s2 @ __fp16 > .L2: > vmov s0, s1 @ __fp16 > bx lr > > There should have been a conditional branch there to skip one of the > VMOVs. > This patch fixes this problem by making *movhf_vfp_fp16 unconditional > wherever needed. > > Testing done: Add a new test case and checked for regressions > arm-none-linux-gnueabihf. > > Is this ok for trunk? > This is ok after assuming a bootstrap on arm-none-linux-gnueabihf passes as well. Does this bug appear on the GCC 7 branch? If so, could you please test this patch on that branch as well if so? Thanks, Kyrill > Sudi > > ChangeLog entry are as follow: > > *** gcc/ChangeLog *** > > 2017-11-24 Sudakshina Das <sudi.das@arm.com> > > * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute. > > *** gcc/testsuite/ChangeLog *** > > 2017-11-24 Sudakshina Das <sudi.das@arm.com> > > * gcc.target/arm/armv8_2-fp16-move-2.c: New test.
Hi Kyrill On 27/11/17 12:25, Kyrill Tkachov wrote: > Hi Sudi, > > On 24/11/17 14:57, Sudi Das wrote: >> Hi >> >> For the following test case: >> __fp16 >> test_select (__fp16 a, __fp16 b, __fp16 c) >> { >> return (a < b) ? b : c; >> } >> >> when compiled with -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm >> -mfloat-abi=hard trunk generates wrong code: >> >> test_select: >> @ args = 0, pretend = 0, frame = 0 >> @ frame_needed = 0, uses_anonymous_args = 0 >> @ link register save eliminated. >> vcvtb.f32.f16 s0, s0 >> vcvtb.f32.f16 s15, s1 >> vcmpe.f32 s0, s15 >> vmrs APSR_nzcv, FPSCR >> // <------ No conditional branch! >> vmov s1, s2 @ __fp16 >> .L2: >> vmov s0, s1 @ __fp16 >> bx lr >> >> There should have been a conditional branch there to skip one of the >> VMOVs. >> This patch fixes this problem by making *movhf_vfp_fp16 unconditional >> wherever needed. >> >> Testing done: Add a new test case and checked for regressions >> arm-none-linux-gnueabihf. >> >> Is this ok for trunk? >> > > This is ok after assuming a bootstrap on arm-none-linux-gnueabihf passes > as well. > Does this bug appear on the GCC 7 branch? > If so, could you please test this patch on that branch as well if so? > I have tested the patch and also sent a new patch request for gcc-7 https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02577.html Thanks Sudi > Thanks, > Kyrill > >> Sudi >> >> ChangeLog entry are as follow: >> >> *** gcc/ChangeLog *** >> >> 2017-11-24 Sudakshina Das <sudi.das@arm.com> >> >> * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute. >> >> *** gcc/testsuite/ChangeLog *** >> >> 2017-11-24 Sudakshina Das <sudi.das@arm.com> >> >> * gcc.target/arm/armv8_2-fp16-move-2.c: New test. >
On 30/11/17 16:06, Sudakshina Das wrote: > Hi Kyrill > > On 27/11/17 12:25, Kyrill Tkachov wrote: > > Hi Sudi, > > > > On 24/11/17 14:57, Sudi Das wrote: > >> Hi > >> > >> For the following test case: > >> __fp16 > >> test_select (__fp16 a, __fp16 b, __fp16 c) > >> { > >> return (a < b) ? b : c; > >> } > >> > >> when compiled with -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm > >> -mfloat-abi=hard trunk generates wrong code: > >> > >> test_select: > >> @ args = 0, pretend = 0, frame = 0 > >> @ frame_needed = 0, uses_anonymous_args = 0 > >> @ link register save eliminated. > >> vcvtb.f32.f16 s0, s0 > >> vcvtb.f32.f16 s15, s1 > >> vcmpe.f32 s0, s15 > >> vmrs APSR_nzcv, FPSCR > >> // <------ No conditional branch! > >> vmov s1, s2 @ __fp16 > >> .L2: > >> vmov s0, s1 @ __fp16 > >> bx lr > >> > >> There should have been a conditional branch there to skip one of the > >> VMOVs. > >> This patch fixes this problem by making *movhf_vfp_fp16 unconditional > >> wherever needed. > >> > >> Testing done: Add a new test case and checked for regressions > >> arm-none-linux-gnueabihf. > >> > >> Is this ok for trunk? > >> > > > > This is ok after assuming a bootstrap on arm-none-linux-gnueabihf passes > > as well. > > Does this bug appear on the GCC 7 branch? > > If so, could you please test this patch on that branch as well if so? > > > > I have tested the patch and also sent a new patch request for gcc-7 > https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02577.html > Thanks Sudi, this is ok to commit to the branch after we let this patch bake on trunk for a week without problems. Kyrill > Thanks > Sudi > > > Thanks, > > Kyrill > > > >> Sudi > >> > >> ChangeLog entry are as follow: > >> > >> *** gcc/ChangeLog *** > >> > >> 2017-11-24 Sudakshina Das <sudi.das@arm.com> > >> > >> * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute. > >> > >> *** gcc/testsuite/ChangeLog *** > >> > >> 2017-11-24 Sudakshina Das <sudi.das@arm.com> > >> > >> * gcc.target/arm/armv8_2-fp16-move-2.c: New test. > > >
On 30/11/17 16:07, Kyrill Tkachov wrote: > > On 30/11/17 16:06, Sudakshina Das wrote: >> Hi Kyrill >> >> On 27/11/17 12:25, Kyrill Tkachov wrote: >> > Hi Sudi, >> > >> > On 24/11/17 14:57, Sudi Das wrote: >> >> Hi >> >> >> >> For the following test case: >> >> __fp16 >> >> test_select (__fp16 a, __fp16 b, __fp16 c) >> >> { >> >> return (a < b) ? b : c; >> >> } >> >> >> >> when compiled with -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm >> >> -mfloat-abi=hard trunk generates wrong code: >> >> >> >> test_select: >> >> @ args = 0, pretend = 0, frame = 0 >> >> @ frame_needed = 0, uses_anonymous_args = 0 >> >> @ link register save eliminated. >> >> vcvtb.f32.f16 s0, s0 >> >> vcvtb.f32.f16 s15, s1 >> >> vcmpe.f32 s0, s15 >> >> vmrs APSR_nzcv, FPSCR >> >> // <------ No conditional branch! >> >> vmov s1, s2 @ __fp16 >> >> .L2: >> >> vmov s0, s1 @ __fp16 >> >> bx lr >> >> >> >> There should have been a conditional branch there to skip one of the >> >> VMOVs. >> >> This patch fixes this problem by making *movhf_vfp_fp16 unconditional >> >> wherever needed. >> >> >> >> Testing done: Add a new test case and checked for regressions >> >> arm-none-linux-gnueabihf. >> >> >> >> Is this ok for trunk? >> >> >> > >> > This is ok after assuming a bootstrap on arm-none-linux-gnueabihf >> passes >> > as well. >> > Does this bug appear on the GCC 7 branch? >> > If so, could you please test this patch on that branch as well if so? >> > >> >> I have tested the patch and also sent a new patch request for gcc-7 >> https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02577.html >> > > Thanks Sudi, this is ok to commit to the branch after we let this patch > bake on trunk for a week without problems. > Committed as r255301 on trunk. Will wait for a week before committing to gcc-7. Thanks Sudi > Kyrill > > >> Thanks >> Sudi >> >> > Thanks, >> > Kyrill >> > >> >> Sudi >> >> >> >> ChangeLog entry are as follow: >> >> >> >> *** gcc/ChangeLog *** >> >> >> >> 2017-11-24 Sudakshina Das <sudi.das@arm.com> >> >> >> >> * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute. >> >> >> >> *** gcc/testsuite/ChangeLog *** >> >> >> >> 2017-11-24 Sudakshina Das <sudi.das@arm.com> >> >> >> >> * gcc.target/arm/armv8_2-fp16-move-2.c: New test. >> > >> >
diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md index 075a938..61b6477 100644 --- a/gcc/config/arm/vfp.md +++ b/gcc/config/arm/vfp.md @@ -410,7 +410,10 @@ gcc_unreachable (); } } - [(set_attr "predicable" "yes, yes, no, yes, no, no, no, no, no, no") + [(set_attr "conds" "*, *, unconditional, *, unconditional, unconditional,\ + unconditional, unconditional, unconditional,\ + unconditional") + (set_attr "predicable" "yes, yes, no, yes, no, no, no, no, no, no") (set_attr "predicable_short_it" "no, no, no, yes,\ no, no, no, no,\ no, no") diff --git a/gcc/testsuite/gcc.target/arm/armv8_2-fp16-move-2.c b/gcc/testsuite/gcc.target/arm/armv8_2-fp16-move-2.c new file mode 100644 index 0000000..fcb857f --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/armv8_2-fp16-move-2.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target arm_v8_2a_fp16_scalar_ok } */ +/* { dg-options "-O2 -marm" } */ +/* { dg-add-options arm_v8_2a_fp16_scalar } */ + +__fp16 +test_select (__fp16 a, __fp16 b, __fp16 c) +{ + return (a < b) ? b : c; +} +/* { dg-final { scan-assembler "bmi" } } */
Hi For the following test case: __fp16 test_select (__fp16 a, __fp16 b, __fp16 c) { return (a < b) ? b : c; } when compiled with -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm -mfloat-abi=hard trunk generates wrong code: test_select: @ args = 0, pretend = 0, frame = 0 @ frame_needed = 0, uses_anonymous_args = 0 @ link register save eliminated. vcvtb.f32.f16 s0, s0 vcvtb.f32.f16 s15, s1 vcmpe.f32 s0, s15 vmrs APSR_nzcv, FPSCR // <------ No conditional branch! vmov s1, s2 @ __fp16 .L2: vmov s0, s1 @ __fp16 bx lr There should have been a conditional branch there to skip one of the VMOVs. This patch fixes this problem by making *movhf_vfp_fp16 unconditional wherever needed. Testing done: Add a new test case and checked for regressions arm-none-linux-gnueabihf. Is this ok for trunk? Sudi ChangeLog entry are as follow: *** gcc/ChangeLog *** 2017-11-24 Sudakshina Das <sudi.das@arm.com> * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute. *** gcc/testsuite/ChangeLog *** 2017-11-24 Sudakshina Das <sudi.das@arm.com> * gcc.target/arm/armv8_2-fp16-move-2.c: New test.