[ARM] Fix invalid instructions generated for data movement.

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.

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

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!