Re: [PATCH] RISC-V: Optimize relax of GP with max_alignment.

[Nelson Chu <nelson@rivosinc.com>]

Unfortunately, I got lots of gcc failures for rv64gc-elf regression,

               ========= Summary of gcc testsuite =========

                            | # of unexpected case / # of unique unexpected case

                            |          gcc |          g++ |     gfortran |

     rv64gc/  lp64d/ medlow |38068 /  4059 |20265 /  2738 |      - |

And I guess that is because we should pass "sec" rather than "sym_sec"
to the _bfd_riscv_get_max_alignment_in_gp.

Besides, not really sure if that may happen: do some sections that
don't be considered in the range of gp in the first round, but will be
considered in the later relax passes?  If so, then does that make the
max_alignment returned by the _bfd_riscv_get_max_alignment_in_gp have
different values?  Not really sure if that will break something.  An
idea is that - we probably can reserve the original max_alignment when
determine if the section is in the range of gp, like this,
https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L4566.
Though it seems a bit weird to estimate the alignment twice, but the
original max alignment is used to estimate and try to cover most of
the sections in the range of gp, and then we will get the new max
alignment, which should be always smaller than the original one, and
use it to estimate if the symbol is in the range if gp.  However, I
don't have any test case for now to approve what I am thinking, but
that at least works for your testcase, too.

Moreover, not sure if we can estimate the new max alignment for gp
once also in here,
https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L4708.
If we just use the original max alignment to estimate it, then doing
once is probably enough.

Happy to see any thoughts!

Thanks
Nelson

On Fri, Sep 23, 2022 at 5:47 PM lifang_xia--- via Binutils
<binutils@sourceware.org> wrote:
>
> From: Lifang Xia <lifang_xia@linux.alibaba.com>
>
> The max_alignment defined out of [gp-2K, gp+2k), the max_alignment
> shouldn't affect the relax of gp.
> If the symbol is in [gp-2K, gp+2k), the max_alignment would be
> replaced with the max_alignment of the section in [gp-2k, gp+2k).
>
> bfd/
>         * elfnn-riscv.c (_bfd_riscv_get_max_alignment_in_gp): New.
>         (_bfd_riscv_relax_lui): The max_alignment of sections is from
>         [gp-2K, gp+2K).
>         (_bfd_riscv_relax_pc): Likewise.
> ld/
>         * ld/testsuite/ld-riscv-elf/relax-max-align.*: New tests.
> ---
>  bfd/elfnn-riscv.c                           | 37 +++++++++++++++++
>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp  |  1 +
>  ld/testsuite/ld-riscv-elf/relax-max-align.d | 46 +++++++++++++++++++++
>  ld/testsuite/ld-riscv-elf/relax-max-align.s | 29 +++++++++++++
>  4 files changed, 113 insertions(+)
>  create mode 100644 ld/testsuite/ld-riscv-elf/relax-max-align.d
>  create mode 100644 ld/testsuite/ld-riscv-elf/relax-max-align.s
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 0e0a0b09e24..83afc95ccab 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -4259,6 +4259,29 @@ _bfd_riscv_get_max_alignment (asection *sec)
>    return (bfd_vma) 1 << max_alignment_power;
>  }
>
> +/* Traverse all output sections and return the max alignment
> +   in [gp-2K, gp+2K) .  */
> +
> +static bfd_vma
> +_bfd_riscv_get_max_alignment_in_gp (asection *sec, bfd_vma gp)
> +{
> +  unsigned int max_alignment_power = 0;
> +  asection *o;
> +
> +  if (sec == NULL)
> +    return 0;
> +
> +  for (o = sec->output_section->owner->sections; o != NULL; o = o->next)
> +    {
> +      if ((VALID_ITYPE_IMM (sec_addr(o) - gp)
> +          || VALID_ITYPE_IMM (sec_addr(o) + o->size - gp))
> +         && (o->alignment_power > max_alignment_power))
> +       max_alignment_power = o->alignment_power;
> +    }
> +
> +  return (bfd_vma) 1 << max_alignment_power;
> +}
> +
>  /* Relax non-PIC global variable references to GP-relative references.  */
>
>  static bool
> @@ -4290,6 +4313,13 @@ _bfd_riscv_relax_lui (bfd *abfd,
>        if (h->u.def.section->output_section == sym_sec->output_section
>           && sym_sec->output_section != bfd_abs_section_ptr)
>         max_alignment = (bfd_vma) 1 << sym_sec->output_section->alignment_power;
> +      else if (!undefined_weak)
> +       {
> +         /* Otherwise, consider the alignment of sections in [gp-2K,gp+2K). */
> +         bfd_vma new_max_alignment = _bfd_riscv_get_max_alignment_in_gp (sym_sec, gp);
> +         if (new_max_alignment)
> +           max_alignment = new_max_alignment;
> +       }
>      }
>
>    /* Is the reference in range of x0 or gp?
> @@ -4556,6 +4586,13 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
>        if (h->u.def.section->output_section == sym_sec->output_section
>           && sym_sec->output_section != bfd_abs_section_ptr)
>         max_alignment = (bfd_vma) 1 << sym_sec->output_section->alignment_power;
> +      else if (!undefined_weak)
> +       {
> +         /* Otherwise, consider the alignment of sections in [gp-2K,gp+2K). */
> +         bfd_vma new_max_alignment = _bfd_riscv_get_max_alignment_in_gp (sym_sec, gp);
> +         if (new_max_alignment)
> +           max_alignment = new_max_alignment;
> +       }
>      }
>
>    /* Is the reference in range of x0 or gp?
> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> index df89e0ee68b..e53f16facfa 100644
> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> @@ -169,6 +169,7 @@ if [istarget "riscv*-*-*"] {
>      run_dump_test "attr-merge-priv-spec-failed-05"
>      run_dump_test "attr-merge-priv-spec-failed-06"
>      run_dump_test "attr-phdr"
> +    run_dump_test "relax-max-align"
>      run_ld_link_tests [list \
>         [list "Weak reference 32" "-T weakref.ld -m[riscv_choose_ilp32_emul]" "" \
>             "-march=rv32i -mabi=ilp32" {weakref32.s} \
> diff --git a/ld/testsuite/ld-riscv-elf/relax-max-align.d b/ld/testsuite/ld-riscv-elf/relax-max-align.d
> new file mode 100644
> index 00000000000..4ba920da970
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/relax-max-align.d
> @@ -0,0 +1,46 @@
> +#source: relax-max-align.s
> +#ld:
> +#objdump: -d
> +
> +.*:[   ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+[0-9a-f]+ <_start>:
> +.*:[   ]+[0-9a-f]+[    ]+addi[         ]+.*<gdata>
> +.*:[   ]+[0-9a-f]+[    ]+jal[  ]+.*
> +.*:[   ]+[0-9a-f]+[    ]+j[    ]+.*
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +.*:[   ]+[0-9a-f]+[    ]+nop
> +
> +0+[0-9a-f]+ <func>:
> +.*:[   ]+[0-9a-f]+[    ]+ret
> +[      ]+...
> diff --git a/ld/testsuite/ld-riscv-elf/relax-max-align.s b/ld/testsuite/ld-riscv-elf/relax-max-align.s
> new file mode 100644
> index 00000000000..0d162ff4d93
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/relax-max-align.s
> @@ -0,0 +1,29 @@
> +
> +.global _start
> +_start:
> +       lui   a0, %hi(gdata)
> +       addi  a0, a0, %lo(gdata)
> +       call func
> +       j .
> +       .size _start, . - _start
> +
> +.global func
> +.align 7
> +func:
> +       ret
> +       .size func, . - func
> +
> +.data
> +padding:
> +       .long 0
> +       .long 0
> +       .long 0
> +       .long 0
> +       .size padding, . - padding
> +
> +.global gdata
> +.type gdata, object
> +gdata:
> +       .zero 4
> +       .size gdata, . - gdata
> +
> --
> 2.32.1 (Apple Git-133)
>