* [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).