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

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

On 10 July 2014 10:50, Jiong Wang <jiong.wang@arm.com> wrote:
>
> On 10/07/14 10:47, Will Newton wrote:
>>
>> 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.
>
>
> Hi Will,
>
>   Thanks for testing.
>
>   Actually, I've done 32-bit host test on chromebook.  I'll double check
> this.

On Ubuntu Trusty:

gcc -DHAVE_CONFIG_H -I. -I../../binutils-gdb/gas  -I.
-I../../binutils-gdb/gas -I../bfd -I../../binutils-gdb/gas/config
-I../../binutils-gdb/gas/../include -I../../binutils-gdb/gas/..
-I../../binutils-gdb/gas/../bfd
-DLOCALEDIR="\"/opt/binutils/share/locale\""  -W -Wall
-Wstrict-prototypes -Wmissing-prototypes -Wshadow -Werror -g -O2 -MT
tc-arm.o -MD -MP -MF .deps/tc-arm.Tpo -c -o tc-arm.o `test -f
'config/tc-arm.c' || echo '../../binutils-gdb/gas/'`config/tc-arm.c
../../binutils-gdb/gas/config/tc-arm.c: In function âadd_to_lit_poolâ:
../../binutils-gdb/gas/config/tc-arm.c:3240:48: error: comparison
between signed and unsigned integer expressions [-Werror=sign-compare]
         && (pool->literals[entry].X_add_number == imm1)
                                                ^
../../binutils-gdb/gas/config/tc-arm.c:3244:52: error: comparison
between signed and unsigned integer expressions [-Werror=sign-compare]
         && (pool->literals[entry + 1].X_add_number == imm2)


-- 
Will Newton
Toolchain Working Group, Linaro