* [ARM] Fix invalid instructions generated for data movement.
@ 2016-09-27 13:23 Matthew Wahab
2016-09-27 13:58 ` Kyrill Tkachov
0 siblings, 1 reply; 3+ messages in thread
From: Matthew Wahab @ 2016-09-27 13:23 UTC (permalink / raw)
To: gcc-patches; +Cc: Christophe Lyon
[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]
Recently added support for ARMv8.2-A
(https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01240.html) included a
number of changes to improve data movement, particularly for HI and HF
mode values. These included the use of the Thumb-2 instruction MOVW and
of the new VMOV.F16 instruction. There are problems with both: the use
of MOVW isn't properly guarded so that it can be generated for targets
that don't support it and the VMOV.F16 instruction is wrongly marked as
being predicable.
This patch adds guards to the use of the MOVW so that it is only
generated when the target supports Thumb-2 and fixes the predication on
the VMOV.F16 instruction.
Tested for arm-none-linux-gnueabihf with native bootstrap and make check
on ARMv8-A and for arm-none-eabi with cross compiled check-gcc on an
ARMv8.2-A emulator.
There is one failure on arm-none-linux-gnueabihf,
gcc.dg/guality/pr36728-1.c which is due to MOVW not being generated,
even for ARMv7-A. The generated code is otherwise correct. I think I
understand why MOVW isn't being emitted but it'll take time to test
properly.
Since this patch is to fix a broken build is it OK to commit it and to fix
the poor code-gen in a follow-up patch?
Matthew
gcc/
2016-09-27 Matthew Wahab <matthew.wahab@arm.com>
* config/arm/arm.md (*arm_movsi_insn): Add "arch" attribute.
* config/arm/vfp.md (*arm_movhi_vfp): Likewise.
(*thumb2_movhi_vfp): Likewise.
(*arm_movhi_fp16): Remove predication operand from VMOV.F16
template. Expand predicable attribute to mark VMOV.F16 as not
predicable. Add "arch" attribute.
(*thumb2_movhi_fp16): Likewise.
(*arm_movsi_vfp): Break a long line. Add "arch" attribute.
(*thumb2_movsi_vfp): Add "arch" attribute.
[-- Attachment #2: 0001-ARM-Fix-invalid-instructions-generated-for-data-move.patch --]
[-- Type: text/x-patch, Size: 5750 bytes --]
From bedba58f504ef1f68ee0f90d9e34563b75653ae5 Mon Sep 17 00:00:00 2001
From: Matthew Wahab <matthew.wahab@arm.com>
Date: Mon, 26 Sep 2016 12:05:34 +0100
Subject: [PATCH] [ARM] Fix invalid instructions generated for data movement.
Recently added support for ARMv8.2-A
(https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01240.html) included a
number of changes to improve data movement, particularly for HI and HF
mode values. These included the use of the Thumb-2 instruction MOVW and
of the new VMOV.F16 instruction. There are problems with both: the use
of MOVW isn't properly guarded so that it can be generated for targets
that don't support it. The VMOV.F16 instruction is wrongly marked as
being predicable.
This patch adds guards to the use of the MOVW so that it is only
generated when the target supports Thumb-2 and fixes the predication on
the VMOV.F16 instruction.
Tested for arm-none-linux-gnueabihf with native bootstrap and make check
on ARMv8-A and for arm-none-eabi with cross compiled check-gcc on an
ARMv8.2-A emulator.
There is one failure on arm-none-linux-gnueabihf,
gcc.dg/guality/pr36728-1.c which is due to MOVW not being generated,
even for ARMv7-A. The generated code is otherwise correct. I think I
understand why MOVW isn't being emitted but it'll take time to test
properly.
Since this patch is to fix a broken build is it OK to commit and to fix
the poor code-gen in a follow-up patch?
gcc/
2016-09-27 Matthew Wahab <matthew.wahab@arm.com>
* config/arm/arm.md (*arm_movsi_insn): Add "arch" attribute.
* config/arm/vfp.md (*arm_movhi_vfp): Likewise.
(*thumb2_movhi_vfP): Likewise.
(*arm_movhi_fp16): Remove predication operand from VMOV.F16
template. Expand predicable attribute to mark VMOV.F16 as not
predicable. Add "arch" attribute.
(*thumb2_movhi_fp16): Likewise.
(*arm_movsi_vfp): Break a long line. Add "arch" attribute.
(*thum2_movsi_vfp): Add "arch" attribute.
---
gcc/config/arm/arm.md | 1 +
gcc/config/arm/vfp.md | 18 +++++++++++++-----
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 411754f..999292b 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6065,6 +6065,7 @@
[(set_attr "type" "mov_reg,mov_imm,mvn_imm,mov_imm,load1,store1")
(set_attr "predicable" "yes")
(set_attr "pool_range" "*,*,*,*,4096,*")
+ (set_attr "arch" "*,*,*,t2,*,*")
(set_attr "neg_pool_range" "*,*,*,*,4084,*")]
)
diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
index 5d22c34..21eaf48 100644
--- a/gcc/config/arm/vfp.md
+++ b/gcc/config/arm/vfp.md
@@ -65,6 +65,7 @@
(const_string "f_mcr")
(const_string "f_mrc")
(const_string "fmov")])
+ (set_attr "arch" "*, *, t2, *, *, *, *, *")
(set_attr "pool_range" "*, *, *, *, 256, *, *, *")
(set_attr "neg_pool_range" "*, *, *, *, 244, *, *, *")
(set_attr "length" "4")]
@@ -108,6 +109,7 @@
(set_attr "type"
"mov_reg, mov_imm, mov_imm, mov_imm, store1, load1,\
f_mcr, f_mrc, fmov")
+ (set_attr "arch" "*, *, *, t2, *, *, *, *, *")
(set_attr "pool_range" "*, *, *, *, *, 4094, *, *, *")
(set_attr "neg_pool_range" "*, *, *, *, *, 250, *, *, *")
(set_attr "length" "2, 4, 2, 4, 4, 4, 4, 4, 4")]
@@ -139,14 +141,14 @@
return "ldrh%?\t%0, %1\t%@ movhi";
case 5:
case 6:
- return "vmov%?.f16\t%0, %1\t%@ int";
+ return "vmov.f16\t%0, %1\t%@ int";
case 7:
return "vmov%?.f32\t%0, %1\t%@ int";
default:
gcc_unreachable ();
}
}
- [(set_attr "predicable" "yes")
+ [(set_attr "predicable" "yes, yes, yes, yes, yes, no, no, yes")
(set_attr_alternative "type"
[(if_then_else
(match_operand 1 "const_int_operand" "")
@@ -159,6 +161,7 @@
(const_string "f_mcr")
(const_string "f_mrc")
(const_string "fmov")])
+ (set_attr "arch" "*, *, t2, *, *, *, *, *")
(set_attr "pool_range" "*, *, *, *, 256, *, *, *")
(set_attr "neg_pool_range" "*, *, *, *, 244, *, *, *")
(set_attr "length" "4")]
@@ -188,19 +191,21 @@
return "ldrh%?\t%0, %1\t%@ movhi";
case 6:
case 7:
- return "vmov%?.f16\t%0, %1\t%@ int";
+ return "vmov.f16\t%0, %1\t%@ int";
case 8:
return "vmov%?.f32\t%0, %1\t%@ int";
default:
gcc_unreachable ();
}
}
- [(set_attr "predicable" "yes")
+ [(set_attr "predicable"
+ "yes, yes, yes, yes, yes, yes, no, no, yes")
(set_attr "predicable_short_it"
"yes, no, yes, no, no, no, no, no, no")
(set_attr "type"
"mov_reg, mov_imm, mov_imm, mov_imm, store1, load1,\
f_mcr, f_mrc, fmov")
+ (set_attr "arch" "*, *, *, t2, *, *, *, *, *")
(set_attr "pool_range" "*, *, *, *, *, 4094, *, *, *")
(set_attr "neg_pool_range" "*, *, *, *, *, 250, *, *, *")
(set_attr "length" "2, 4, 2, 4, 4, 4, 4, 4, 4")]
@@ -241,7 +246,9 @@
}
"
[(set_attr "predicable" "yes")
- (set_attr "type" "mov_reg,mov_reg,mvn_imm,mov_imm,load1,store1,f_mcr,f_mrc,fmov,f_loads,f_stores")
+ (set_attr "type" "mov_reg,mov_reg,mvn_imm,mov_imm,load1,store1,
+ f_mcr,f_mrc,fmov,f_loads,f_stores")
+ (set_attr "arch" "*,*,*,t2,*,*,*,*,*,*,*")
(set_attr "pool_range" "*,*,*,*,4096,*,*,*,*,1020,*")
(set_attr "neg_pool_range" "*,*,*,*,4084,*,*,*,*,1008,*")]
)
@@ -290,6 +297,7 @@
(set_attr "predicable_short_it" "yes,no,yes,no,no,no,no,no,no,no,no,no,no,no")
(set_attr "type" "mov_reg,mov_reg,mov_reg,mvn_reg,mov_imm,load1,load1,store1,store1,f_mcr,f_mrc,fmov,f_loads,f_stores")
(set_attr "length" "2,4,2,4,4,4,4,4,4,4,4,4,4,4")
+ (set_attr "arch" "*,*,*,*,t2,*,*,*,*,*,*,*,*,*")
(set_attr "pool_range" "*,*,*,*,*,1018,4094,*,*,*,*,*,1018,*")
(set_attr "neg_pool_range" "*,*,*,*,*, 0, 0,*,*,*,*,*,1008,*")]
)
--
2.1.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [ARM] Fix invalid instructions generated for data movement.
2016-09-27 13:23 [ARM] Fix invalid instructions generated for data movement Matthew Wahab
@ 2016-09-27 13:58 ` Kyrill Tkachov
2016-09-27 18:31 ` Christophe Lyon
0 siblings, 1 reply; 3+ messages in thread
From: Kyrill Tkachov @ 2016-09-27 13:58 UTC (permalink / raw)
To: Matthew Wahab, gcc-patches; +Cc: Christophe Lyon
Hi Matthew,
On 27/09/16 14:21, Matthew Wahab wrote:
> Recently added support for ARMv8.2-A
> (https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01240.html) included a
> number of changes to improve data movement, particularly for HI and HF
> mode values. These included the use of the Thumb-2 instruction MOVW and
> of the new VMOV.F16 instruction. There are problems with both: the use
> of MOVW isn't properly guarded so that it can be generated for targets
> that don't support it and the VMOV.F16 instruction is wrongly marked as
> being predicable.
>
> This patch adds guards to the use of the MOVW so that it is only
> generated when the target supports Thumb-2 and fixes the predication on
> the VMOV.F16 instruction.
>
> Tested for arm-none-linux-gnueabihf with native bootstrap and make check
> on ARMv8-A and for arm-none-eabi with cross compiled check-gcc on an
> ARMv8.2-A emulator.
>
> There is one failure on arm-none-linux-gnueabihf,
> gcc.dg/guality/pr36728-1.c which is due to MOVW not being generated,
> even for ARMv7-A. The generated code is otherwise correct. I think I
> understand why MOVW isn't being emitted but it'll take time to test
> properly.
>
> Since this patch is to fix a broken build is it OK to commit it and to fix
> the poor code-gen in a follow-up patch?
> Matthew
>
> gcc/
> 2016-09-27 Matthew Wahab <matthew.wahab@arm.com>
>
> * config/arm/arm.md (*arm_movsi_insn): Add "arch" attribute.
> * config/arm/vfp.md (*arm_movhi_vfp): Likewise.
> (*thumb2_movhi_vfp): Likewise.
> (*arm_movhi_fp16): Remove predication operand from VMOV.F16
> template. Expand predicable attribute to mark VMOV.F16 as not
> predicable. Add "arch" attribute.
> (*thumb2_movhi_fp16): Likewise.
> (*arm_movsi_vfp): Break a long line. Add "arch" attribute.
> (*thumb2_movsi_vfp): Add "arch" attribute.
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 411754f..999292b 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6065,6 +6065,7 @@
[(set_attr "type" "mov_reg,mov_imm,mvn_imm,mov_imm,load1,store1")
(set_attr "predicable" "yes")
(set_attr "pool_range" "*,*,*,*,4096,*")
+ (set_attr "arch" "*,*,*,t2,*,*")
(set_attr "neg_pool_range" "*,*,*,*,4084,*")]
)
The "t2" attribute means that we're currently compiling for Thumb2. What you want is to check that the architecture
we're compiling for supports Thumb2, so in this case you want the v6t2 value.
In the interest of fixing the build I'll approve this patch as is since it's still correct (just not as general as it could be),
but it'd be good to have a follow-up patch to fix this.
Thanks for fixing this,
Kyrill
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [ARM] Fix invalid instructions generated for data movement.
2016-09-27 13:58 ` Kyrill Tkachov
@ 2016-09-27 18:31 ` Christophe Lyon
0 siblings, 0 replies; 3+ messages in thread
From: Christophe Lyon @ 2016-09-27 18:31 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: Matthew Wahab, gcc-patches
Hi,
On 27 September 2016 at 15:55, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi Matthew,
>
>
> On 27/09/16 14:21, Matthew Wahab wrote:
>>
>> Recently added support for ARMv8.2-A
>> (https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01240.html) included a
>> number of changes to improve data movement, particularly for HI and HF
>> mode values. These included the use of the Thumb-2 instruction MOVW and
>> of the new VMOV.F16 instruction. There are problems with both: the use
>> of MOVW isn't properly guarded so that it can be generated for targets
>> that don't support it and the VMOV.F16 instruction is wrongly marked as
>> being predicable.
>>
>> This patch adds guards to the use of the MOVW so that it is only
>> generated when the target supports Thumb-2 and fixes the predication on
>> the VMOV.F16 instruction.
>>
>> Tested for arm-none-linux-gnueabihf with native bootstrap and make check
>> on ARMv8-A and for arm-none-eabi with cross compiled check-gcc on an
>> ARMv8.2-A emulator.
>>
>> There is one failure on arm-none-linux-gnueabihf,
>> gcc.dg/guality/pr36728-1.c which is due to MOVW not being generated,
>> even for ARMv7-A. The generated code is otherwise correct. I think I
>> understand why MOVW isn't being emitted but it'll take time to test
>> properly.
>>
>> Since this patch is to fix a broken build is it OK to commit it and to fix
>> the poor code-gen in a follow-up patch?
>> Matthew
>>
>> gcc/
>> 2016-09-27 Matthew Wahab <matthew.wahab@arm.com>
>>
>> * config/arm/arm.md (*arm_movsi_insn): Add "arch" attribute.
>> * config/arm/vfp.md (*arm_movhi_vfp): Likewise.
>> (*thumb2_movhi_vfp): Likewise.
>> (*arm_movhi_fp16): Remove predication operand from VMOV.F16
>> template. Expand predicable attribute to mark VMOV.F16 as not
>> predicable. Add "arch" attribute.
>> (*thumb2_movhi_fp16): Likewise.
>> (*arm_movsi_vfp): Break a long line. Add "arch" attribute.
>> (*thumb2_movsi_vfp): Add "arch" attribute.
>
>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 411754f..999292b 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -6065,6 +6065,7 @@
> [(set_attr "type" "mov_reg,mov_imm,mvn_imm,mov_imm,load1,store1")
> (set_attr "predicable" "yes")
> (set_attr "pool_range" "*,*,*,*,4096,*")
> + (set_attr "arch" "*,*,*,t2,*,*")
> (set_attr "neg_pool_range" "*,*,*,*,4084,*")]
> )
>
> The "t2" attribute means that we're currently compiling for Thumb2. What you
> want is to check that the architecture
> we're compiling for supports Thumb2, so in this case you want the v6t2
> value.
> In the interest of fixing the build I'll approve this patch as is since it's
> still correct (just not as general as it could be),
> but it'd be good to have a follow-up patch to fix this.
>gcc.target/arm/constant-pool.c execution test
> Thanks for fixing this,
> Kyrill
>
>
This patch fixes the regression I reported on
gcc.target/arm/constant-pool.c
gfortran.fortran-torture/execute/nan_inf_fmt.f90
as well as the compiler build failure I reported
against patch 6/17 of your series.
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-09-27 18:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-27 13:23 [ARM] Fix invalid instructions generated for data movement Matthew Wahab
2016-09-27 13:58 ` Kyrill Tkachov
2016-09-27 18:31 ` Christophe Lyon
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).