[PATCH][ARM] Fix wrong code by arm_final_prescan with fp16 move instructions

[PATCH][ARM] Fix wrong code by arm_final_prescan with fp16 move instructions

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

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.

[-- Attachment #2: patch-7703.diff --]
[-- Type: text/x-patch, Size: 1161 bytes --]

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" } } */

Re: [PATCH][ARM] Fix wrong code by arm_final_prescan with fp16 move instructions

[Kyrill Tkachov]

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.

Re: [PATCH][ARM] Fix wrong code by arm_final_prescan with fp16 move instructions

[Sudakshina Das]

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.
>

Re: [PATCH][ARM] Fix wrong code by arm_final_prescan with fp16 move instructions

[Kyrill Tkachov]

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.
> >
>

Re: [PATCH][ARM] Fix wrong code by arm_final_prescan with fp16 move instructions

[Sudakshina Das]

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.
>> >
>>
>