public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][ARM][gcc-7] Fix wrong code by arm_final_prescan with fp16 move instructions
@ 2017-11-30 16:07 Sudakshina Das
  2017-12-11 17:12 ` Sudakshina Das
  0 siblings, 1 reply; 4+ messages in thread
From: Sudakshina Das @ 2017-11-30 16:07 UTC (permalink / raw)
  To: gcc-patches; +Cc: nd, kyrylo.tkachov, Richard Earnshaw, Ramana Radhakrishnan

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

Hi

This patch is the fix for gcc-7 for the same issue as mentioned in:
https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html


For the following test case:
__fp16
test_select (__fp16 a, __fp16 b, __fp16 c)
{
   return (a < b) ? b : c;
}

when compiled with -O2 -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm 
-mfloat-abi=hard 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
	vmov.f16	r3, s2	@ __fp16
	vcmpe.f32	s0, s15
	vmrs	APSR_nzcv, FPSCR
         // <------ No conditional branch
	vmov.f16	r3, s1	@ __fp16
.L1:
	vmov.f16	s0, r3	@ __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.

Testing done: Add a new test case and checked for regressions on 
bootstrapped arm-none-linux-gnueabihf.

Is this ok for gcc-7?

Sudi

ChangeLog entry are as follow:

*** gcc/ChangeLog ***

2017-11-30  Sudakshina Das  <sudi.das@arm.com>

	* config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute.

*** gcc/testsuite/ChangeLog ***

2017-11-30  Sudakshina Das  <sudi.das@arm.com>

	* gcc.target/arm/armv8_2-fp16-move-2.c: New test.

[-- Attachment #2: rb8606.patch --]
[-- Type: text/x-patch, Size: 1347 bytes --]

diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
index d8f77e2ffe4fdb7c952d6a5ac947d91f89ce259d..9f06c3da9526d09e43836a60f14da9a49671a393 100644
--- a/gcc/config/arm/vfp.md
+++ b/gcc/config/arm/vfp.md
@@ -456,7 +456,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 0000000000000000000000000000000000000000..ac7d4e3f2a9fb1d70a9ce95062dc6db4a69272ff
--- /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 -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm -mfloat-abi=hard" }  */
+/* { 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 "bpl" } } */

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][ARM][gcc-7] Fix wrong code by arm_final_prescan with fp16 move instructions
  2017-11-30 16:07 [PATCH][ARM][gcc-7] Fix wrong code by arm_final_prescan with fp16 move instructions Sudakshina Das
@ 2017-12-11 17:12 ` Sudakshina Das
  2017-12-12  9:59   ` Christophe Lyon
  0 siblings, 1 reply; 4+ messages in thread
From: Sudakshina Das @ 2017-12-11 17:12 UTC (permalink / raw)
  To: gcc-patches; +Cc: nd, kyrylo.tkachov, Richard Earnshaw, Ramana Radhakrishnan

On 30/11/17 16:01, Sudakshina Das wrote:
> Hi
> 
> This patch is the fix for gcc-7 for the same issue as mentioned in:
> https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html
> 
> 
> For the following test case:
> __fp16
> test_select (__fp16 a, __fp16 b, __fp16 c)
> {
>    return (a < b) ? b : c;
> }
> 
> when compiled with -O2 -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm 
> -mfloat-abi=hard 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
>      vmov.f16    r3, s2    @ __fp16
>      vcmpe.f32    s0, s15
>      vmrs    APSR_nzcv, FPSCR
>          // <------ No conditional branch
>      vmov.f16    r3, s1    @ __fp16
> .L1:
>      vmov.f16    s0, r3    @ __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.
> 
> Testing done: Add a new test case and checked for regressions on 
> bootstrapped arm-none-linux-gnueabihf.
> 
> Is this ok for gcc-7?
> 
> Sudi
> 
> ChangeLog entry are as follow:
> 
> *** gcc/ChangeLog ***
> 
> 2017-11-30  Sudakshina Das  <sudi.das@arm.com>
> 
>      * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2017-11-30  Sudakshina Das  <sudi.das@arm.com>
> 
>      * gcc.target/arm/armv8_2-fp16-move-2.c: New test.

As per the trunk thread for this 
(https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html) committed as 
r255536 on gcc-7-branch for the backport.

Thanks
Sudi

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][ARM][gcc-7] Fix wrong code by arm_final_prescan with fp16 move instructions
  2017-12-11 17:12 ` Sudakshina Das
@ 2017-12-12  9:59   ` Christophe Lyon
  2017-12-12 10:18     ` Sudakshina Das
  0 siblings, 1 reply; 4+ messages in thread
From: Christophe Lyon @ 2017-12-12  9:59 UTC (permalink / raw)
  To: Sudakshina Das
  Cc: gcc-patches, nd, Kyrylo Tkachov, Richard Earnshaw, Ramana Radhakrishnan

Hi,



On 11 December 2017 at 18:12, Sudakshina Das <sudi.das@arm.com> wrote:
> On 30/11/17 16:01, Sudakshina Das wrote:
>>
>> Hi
>>
>> This patch is the fix for gcc-7 for the same issue as mentioned in:
>> https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html
>>
>>
>> For the following test case:
>> __fp16
>> test_select (__fp16 a, __fp16 b, __fp16 c)
>> {
>>    return (a < b) ? b : c;
>> }
>>
>> when compiled with -O2 -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm
>> -mfloat-abi=hard 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
>>      vmov.f16    r3, s2    @ __fp16
>>      vcmpe.f32    s0, s15
>>      vmrs    APSR_nzcv, FPSCR
>>          // <------ No conditional branch
>>      vmov.f16    r3, s1    @ __fp16
>> .L1:
>>      vmov.f16    s0, r3    @ __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.
>>
>> Testing done: Add a new test case and checked for regressions on
>> bootstrapped arm-none-linux-gnueabihf.
>>
>> Is this ok for gcc-7?
>>
>> Sudi
>>
>> ChangeLog entry are as follow:
>>
>> *** gcc/ChangeLog ***
>>
>> 2017-11-30  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2017-11-30  Sudakshina Das  <sudi.das@arm.com>
>>
>>      * gcc.target/arm/armv8_2-fp16-move-2.c: New test.
>
>
> As per the trunk thread for this
> (https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html) committed as
> r255536 on gcc-7-branch for the backport.
>

I've noticed that this backport fails on arm-none-linux-gnueabi and
arm-none-eabi.
I suspect this is partly due to the fact that I use a "recent"
dejagnu, and has to do with whether
dg-add-options are appended or pre-pended.
I'm seeing a compilation line with:

-mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm -mfloat-abi=hard
-mfpu=fp-armv8 -mfloat-abi=softfp -march=armv8.2-a+fp16

leading to:
FAIL: gcc.target/arm/armv8_2-fp16-move-2.c scan-assembler bpl

I'm not sure why this works on trunk, but there I have only:
-marm -mfloat-abi=softfp -march=armv8.2-a+fp16

Maybe this has to do with the new way cpu/fpu options are parsed on trunk.

Christophe


> Thanks
> Sudi

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][ARM][gcc-7] Fix wrong code by arm_final_prescan with fp16 move instructions
  2017-12-12  9:59   ` Christophe Lyon
@ 2017-12-12 10:18     ` Sudakshina Das
  0 siblings, 0 replies; 4+ messages in thread
From: Sudakshina Das @ 2017-12-12 10:18 UTC (permalink / raw)
  To: Christophe Lyon
  Cc: gcc-patches, nd, Kyrylo Tkachov, Richard Earnshaw, Ramana Radhakrishnan

Hi Christophe

On 12/12/17 09:59, Christophe Lyon wrote:
> Hi,
> 
> 
> 
> On 11 December 2017 at 18:12, Sudakshina Das <sudi.das@arm.com> wrote:
>> On 30/11/17 16:01, Sudakshina Das wrote:
>>>
>>> Hi
>>>
>>> This patch is the fix for gcc-7 for the same issue as mentioned in:
>>> https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html
>>>
>>>
>>> For the following test case:
>>> __fp16
>>> test_select (__fp16 a, __fp16 b, __fp16 c)
>>> {
>>>     return (a < b) ? b : c;
>>> }
>>>
>>> when compiled with -O2 -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm
>>> -mfloat-abi=hard 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
>>>       vmov.f16    r3, s2    @ __fp16
>>>       vcmpe.f32    s0, s15
>>>       vmrs    APSR_nzcv, FPSCR
>>>           // <------ No conditional branch
>>>       vmov.f16    r3, s1    @ __fp16
>>> .L1:
>>>       vmov.f16    s0, r3    @ __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.
>>>
>>> Testing done: Add a new test case and checked for regressions on
>>> bootstrapped arm-none-linux-gnueabihf.
>>>
>>> Is this ok for gcc-7?
>>>
>>> Sudi
>>>
>>> ChangeLog entry are as follow:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-11-30  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>       * config/arm/vfp.md (*movhf_vfp_fp16): Add conds attribute.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2017-11-30  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>       * gcc.target/arm/armv8_2-fp16-move-2.c: New test.
>>
>>
>> As per the trunk thread for this
>> (https://gcc.gnu.org/ml/gcc-patches/2017-11/msg02209.html) committed as
>> r255536 on gcc-7-branch for the backport.
>>
> 
> I've noticed that this backport fails on arm-none-linux-gnueabi and
> arm-none-eabi.
> I suspect this is partly due to the fact that I use a "recent"
> dejagnu, and has to do with whether
> dg-add-options are appended or pre-pended.
> I'm seeing a compilation line with:
> 
> -mfpu=fp-armv8 -march=armv8.2-a+fp16 -marm -mfloat-abi=hard
> -mfpu=fp-armv8 -mfloat-abi=softfp -march=armv8.2-a+fp16
> 
> leading to:
> FAIL: gcc.target/arm/armv8_2-fp16-move-2.c scan-assembler bpl
> 
> I'm not sure why this works on trunk, but there I have only:
> -marm -mfloat-abi=softfp -march=armv8.2-a+fp16
> 
> Maybe this has to do with the new way cpu/fpu options are parsed on trunk.

Sorry for this. I will try to investigate.

Thanks
Sudi

> 
> Christophe
> 
> 
>> Thanks
>> Sudi

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-12-12 10:18 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-30 16:07 [PATCH][ARM][gcc-7] Fix wrong code by arm_final_prescan with fp16 move instructions Sudakshina Das
2017-12-11 17:12 ` Sudakshina Das
2017-12-12  9:59   ` Christophe Lyon
2017-12-12 10:18     ` Sudakshina Das

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).