Re: [PATCH/ARM][v2]PR 16722, support VLDR s/d0, =Imm

[Will Newton <will dot newton at linaro dot org>]

On 17 June 2014 11:22, Jiong Wang <jiong.wang@arm.com> wrote:
> This patch addressed all comments at
> https://sourceware.org/ml/binutils/2014-04/msg00263.html
>
> important updates including:
>
> * fix 32-bit host bug.
>   on 32-bit host, X_add_number field in expression is actually 4 byte, thus
> 8 byte
>   constant will go into big number, while this cause trouble. because big
> number
>   is record in global variable "generic_bignum" which will be overrided by
> later
>   assignment.
>
>   To solve this problem, *parse_big_immediate* is invoked *instead of
> my_get_expression*,
>   so 8 byte value is actually splitted and stored in "imm" and "reg" field
> in inst.operands,
>   "regisimm" marked as true, and the value is kept safely to the time pool
> emitted.
>
>   so what this new patch done is
>     * always split 8 byte entry into two 4 byte entries in the pool.
>     * matching existed 8 byte entry still work.
>     * 4 byte entry could reuse half of 8 byte entry is matched.
>
> * alignment
>     * literal pool always start at 8 byte boundary.
>     * 8 byte entry aligned to 8 byte boundary, padding is necessary.
>     * 4 byte entry could utilize any padding existed.
>
> * big-endian
>  * test done
>     * no regression on x86-64 cross arm-none-eabi.
>     * no regression for check-gas/binutils/ld on chromebook native
> arm-linux-gnueabihf.
>     * big-endian also pass one hand-written execution test on qemu.
>
> please review, thanks.
>
> PR target/16722
> gas/
>   * config/tc-arm.c (literal_pool): New field "alignment".
>   (find_or_make_literal_pool): Initialize "alignment" to 2.
>   (s_ltorg): Align the pool using value of "alignment"
>   (parse_big_immediate): New parameter "in_exp". Return
>   parsed expression if "in_exp" is not null.
>   (parse_address_main): Invoke "parse_big_immediate" for
>   constant parameter.
>   (add_to_lit_pool): Add one parameter 'nbytes'.
>   Split 8 byte entry into two 4 byte entry.
>   Add padding to align 8 byte entry to 8 byte boundary.
>   (encode_arm_cp_address): Generate literal pool entry if possible.
>   (move_or_literal_pool): Generate entry for vldr case.
>   (enum lit_type): New enum type.
>   (do_ldst): Use new enum type.
>   (do_ldstv4): Likewise.
>   (do_t_ldst): Likewise.
>   (neon_write_immbits): Support Thumb-2 mode.
>  gas/testsuite/
>   * gas/arm/ldconst.s: Add test cases for symbol literal.
>   * gas/arm/ldconst.d: Likewise.
>   * gas/arm/vldconst.s: Add test cases for vldr.
>   * gas/arm/thumb2_vpool.s: Likewise.
>   * gas/arm/vldconst.d: New pattern for little-endian.
>   * gas/arm/thumb2_vpool.d: Likewise.
>   * gas/arm/vldconst_be.d: New pattern for big-endian.
>   * gas/arm/thumb2_vpool_be.d: Likewise.

Sorry for taking so long to test this, but this breaks when building
on a 32bit platform.

  unsigned imm1;
  unsigned imm2 = 0;

  if (nbytes == 8)
    {
      imm1 = inst.operands[1].imm;
      imm2 = (inst.operands[1].regisimm ? inst.operands[1].reg
           : inst.reloc.exp.X_unsigned ? 0
           : ((int64_t)(imm1)) >> 32);

On 32bit imm1 is a 32bit type, casting it to 64bit will not give any
useful result. I also get signed/unsigned comparison errors comparing
imm1/imm2 to X_add_number.

-- 
Will Newton
Toolchain Working Group, Linaro