Re: [PATCH] [RISC-V] Fix the valid gp range for gp relax.

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 6734 bytes --]

Sorry, I think this patch is not correct.

On Wed, Jul 5, 2023 at 4:42 PM Die Li <lidie@eswincomputing.com> wrote:

> Taking alignment into consideration, the conservative addressing range
> for gp relax is [gp - 12bit_imm + max_alignment, gp + 12bit_imm -
> max_alignment],
>

The max_alignment is used to reserve the space in case the range is
extended because of the alignment.  Therefore, the reserved max_alignment
should make the value between gp and target symbol be larger or smaller, if
it is positive or negative.

Besides, the offset must be based on gp, so no matter whether gp is larger
or smaller than the target symbol, we should always use "symval - gp" as
the offset.

Thanks
Nelson


> which is smaller than the ideal GP addressing range [gp - 12bit_imm, gp +
> 12bit_imm]. Therefore, when symval < gp, it is necessary to ensure that
> both
> symval and symval + reserve_size fall within the range [gp - 12bit_imm +
> alignment,
> gp + 12bit_imm - alignment]. Current linker takes the range of gp
> conservatively,
> this patch allows more symbols to be accessed by gp relative addressing.

Take the file named with gp-relax.s from this patch as the source file.
>
> After assembling and linking with --relax option,
> Before this patch:
> 0+[0-9a-f]+ <_start>:
> .*:[    ]+[0-9a-f]+[    ]+lui+.*
> .*:[    ]+[0-9a-f]+[    ]+addi+.*
> .*:[    ]+[0-9a-f]+[    ]+lui+.*
> .*:[    ]+[0-9a-f]+[    ]+addi+.*
> .*:[    ]+[0-9a-f]+[    ]+auipc+.*
> .*:[    ]+[0-9a-f]+[    ]+lw+.*
>
> Check the symbal table after linking, we have :
> Symbol table '.symtab' contains 19 entries:
>    Num:    Value          Size Type    Bind   Vis      Ndx Name
> ...
>      7: 0000000000011900     0 NOTYPE  GLOBAL DEFAULT  ABS
> __global_pointer$
> ...
>     18: 0000000000011108  3600 OBJECT  GLOBAL DEFAULT    3 global_array
>
> By comparing the values of `global_array` and `__global_pointer$`, the
> symbol of
> global_array can be accessed by gp relative addressing.
>
> After this patch:
> 0+[0-9a-f]+ <_start>:
> .*:[    ]+[0-9a-f]+[    ]+addi[         ]+a3,gp,\-[0-9]+ # [0-9a-f]+
> <global_array>
> .*:[    ]+[0-9a-f]+[    ]+addi[         ]+a4,gp,\-[0-9]+ # [0-9a-f]+
> <global_array+.*
> .*:[    ]+[0-9a-f]+[    ]+lw[   ]+a0,\-[0-9]+\(gp\) # [0-9a-f]+
> <global_array>
>
> Co-Authored by: Fei Gao <gaofei@eswincomputing.com>
> Signed-off-by: Die Li <lidie@eswincomputing.com>
>
> ChangeLog:
>
>         * bfd/elfnn-riscv.c (_bfd_riscv_relax_lui): Fix the valid gp range.
>         (_bfd_riscv_relax_pc): Likewise.
>         * ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: New test.
>         * ld/testsuite/ld-riscv-elf/gp-relax.d: New test.
>         * ld/testsuite/ld-riscv-elf/gp-relax.s: New test.
> ---
>  bfd/elfnn-riscv.c                          |  6 +++--
>  ld/testsuite/ld-riscv-elf/gp-relax.d       | 14 ++++++++++
>  ld/testsuite/ld-riscv-elf/gp-relax.s       | 30 ++++++++++++++++++++++
>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp |  1 +
>  4 files changed, 49 insertions(+), 2 deletions(-)
>  create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax.d
>  create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax.s
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 09aa7be225e..00e692c6924 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -4633,7 +4633,8 @@ _bfd_riscv_relax_lui (bfd *abfd,
>        || (symval >= gp
>           && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
>        || (symval < gp
> -         && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
> +          && VALID_ITYPE_IMM (symval - gp - max_alignment)
> +          && VALID_ITYPE_IMM (symval - gp + max_alignment +
> reserve_size)))
>      {
>        unsigned sym = ELFNN_R_SYM (rel->r_info);
>        switch (ELFNN_R_TYPE (rel->r_info))
> @@ -4897,7 +4898,8 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
>        || (symval >= gp
>           && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
>        || (symval < gp
> -         && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
> +          && VALID_ITYPE_IMM (symval - gp - max_alignment)
> +          && VALID_ITYPE_IMM (symval - gp + max_alignment +
> reserve_size)))
>      {
>        unsigned sym = hi_reloc.hi_sym;
>        switch (ELFNN_R_TYPE (rel->r_info))
> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax.d
> b/ld/testsuite/ld-riscv-elf/gp-relax.d
> new file mode 100644
> index 00000000000..fa93c05ee2c
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/gp-relax.d
> @@ -0,0 +1,14 @@
> +#source: gp-relax.s
> +#as:
> +#ld: --relax
> +#objdump: -d -Mno-aliases
> +
> +.*:[   ]+file format .*
> +
> +
> +Disassembly of section \.text:
> +
> +0+[0-9a-f]+ <_start>:
> +.*:[   ]+[0-9a-f]+[    ]+addi[         ]+a3,gp,\-[0-9]+ # [0-9a-f]+
> <global_array>
> +.*:[   ]+[0-9a-f]+[    ]+addi[         ]+a4,gp,\-[0-9]+ # [0-9a-f]+
> <global_array+.*
> +.*:[   ]+[0-9a-f]+[    ]+lw[   ]+a0,\-[0-9]+\(gp\) # [0-9a-f]+
> <global_array>
> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax.s
> b/ld/testsuite/ld-riscv-elf/gp-relax.s
> new file mode 100644
> index 00000000000..36addbc770c
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/gp-relax.s
> @@ -0,0 +1,30 @@
> +       .globl  global_var
> +       .section        .sbss,"aw",@nobits
> +       .align  2
> +       .type   global_var, @object
> +       .size   global_var, 4
> +global_var:
> +       .zero   4
> +
> +       .globl  global_array
> +       .bss
> +       .align  3
> +       .type   global_array, @object
> +       .size   global_array, 3600
> +global_array:
> +       .zero   3600
> +
> +       .text
> +       .align  1
> +       .globl  _start
> +       .type   _start, @function
> +_start:
> +       lui     a3,%hi(global_array)
> +       addi    a3,a3,%lo(global_array)
> +
> +       lui     a4,%hi(global_array + 0x600)
> +       addi    a4,a4,%lo(global_array + 0x600)
> +
> +.LA0:
> +        auipc  a5,%pcrel_hi(global_array)
> +       lw      a0,%pcrel_lo(.LA0)(a5)
> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> index 947a266ba72..6a04955b23b 100644
> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> @@ -124,6 +124,7 @@ if [istarget "riscv*-*-*"] {
>      run_dump_test "pcgp-relax-01"
>      run_dump_test "pcgp-relax-01-norelaxgp"
>      run_dump_test "pcgp-relax-02"
> +    run_dump_test "gp-relax"
>      run_dump_test "c-lui"
>      run_dump_test "c-lui-2"
>      run_dump_test "disas-jalr"
> --
> 2.17.1
>
>