Re: [ARM] Fix invalid instructions generated for data movement.

[Christophe Lyon <christophe dot lyon at linaro dot org>]

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!