From: Nelson Chu <nelson@rivosinc.com>
To: binutils@sourceware.org, jim.wilson.gcc@gmail.com,
palmer@dabbelt.com, lifang_xia@linux.alibaba.com
Subject: Re: [PATCH] RISC-V: Optimize relaxation of gp with max_alignment.
Date: Fri, 21 Apr 2023 15:50:05 +0800 [thread overview]
Message-ID: <CAPpQWtB=QNQX+FTPhScVJrK0LHUH8Vva3V+uEut=KFYeJXCOVA@mail.gmail.com> (raw)
In-Reply-To: <20230417095443.73581-1-nelson@rivosinc.com>
[-- Attachment #1: Type: text/plain, Size: 15027 bytes --]
Committed, thanks.
Nelson
On Mon, Apr 17, 2023 at 5:54 PM Nelson Chu <nelson@rivosinc.com> wrote:
> From: Lifang Xia <lifang_xia@linux.alibaba.com>
>
> This should be the first related issue, which posted in
> riscv-gnu-toolchain,
> https://github.com/riscv-collab/riscv-gnu-toolchain/issues/497
>
> If the output sections are not between gp and the symbol, then their
> alignments
> shouldn't affect the gp relaxation. However, this patch improves this idea
> even more, it limits the range to the gp+-2k, which means only the output
> section which are in the [gp-2K, gp+2K) range need to be considered.
>
> Even if the output section candidates may be different for each relax
> passes,
> the symbol that can be relaxed ar this round will not be truncated at next
> round. That is because this round you can do relax to indicate that the
> section where the symbol is located is within the [gp-2K, gp+2K) range, so
> all
> the output section alignments between them should be considered. In other
> words, if the alignments between them may cause truncated, then we should
> already preserve the size and won't do the gp relaxation this time.
>
> This patch can resolve the github issue which mentioned above, and also
> passed
> all gcc/binutils regressions of riscv-gnu-toolchain, so should be worth and
> safe enough to commit.
>
> Originally, this patch also do the same optimization for the call
> relaxations,
> https://sourceware.org/pipermail/binutils/2022-October/123918.html
> But just in case there is something that has not been considered, we only
> deal with the gp relaxation at this time.
>
> bfd/
> * elfnn-riscv.c (riscv_elf_link_hash_table): Added new bfd_vma,
> max_alignment_for_gp. It is used to record the maximum alignment
> of
> the output sections, which are in the [gp-2K, gp+2k) range.
> (riscv_elf_link_hash_table_create): Init max_alignment_for_gp to
> -1.
> (_bfd_riscv_get_max_alignment): Added new parameter, gp. If gp is
> zero, then all the output section alignments are possible
> candidates;
> Otherwise, only the output sections which are in the [gp-2K, gp+2K)
> range need to be considered.
> (_bfd_riscv_relax_lui): Called _bfd_riscv_get_max_alignment with
> the
> non-zero gp if the max_alignment_for_gp is -1.
> (_bfd_riscv_relax_pc): Likewise.
> (_bfd_riscv_relax_section): Record the first input section, so that
> we can reset the max_alignment_for_gp for each repeated relax
> passes.
> ld/
> * testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
> * testsuite/ld-riscv-elf/relax-max-align-gp.*: New testcase. It
> fails
> without this patch.
>
> Co-developed-by: Nelson Chu <nelson@rivosinc.com>
> ---
> bfd/elfnn-riscv.c | 103 +++++++++++++-----
> ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 1 +
> .../ld-riscv-elf/relax-max-align-gp.d | 46 ++++++++
> .../ld-riscv-elf/relax-max-align-gp.s | 28 +++++
> 4 files changed, 149 insertions(+), 29 deletions(-)
> create mode 100644 ld/testsuite/ld-riscv-elf/relax-max-align-gp.d
> create mode 100644 ld/testsuite/ld-riscv-elf/relax-max-align-gp.s
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 0dd9b27c8ae..e90e36b58bb 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -200,6 +200,9 @@ struct riscv_elf_link_hash_table
> /* The max alignment of output sections. */
> bfd_vma max_alignment;
>
> + /* The max alignment of output sections in [gp-2K, gp+2K) range. */
> + bfd_vma max_alignment_for_gp;
> +
> /* Used by local STT_GNU_IFUNC symbols. */
> htab_t loc_hash_table;
> void * loc_hash_memory;
> @@ -488,6 +491,7 @@ riscv_elf_link_hash_table_create (bfd *abfd)
> }
>
> ret->max_alignment = (bfd_vma) -1;
> + ret->max_alignment_for_gp = (bfd_vma) -1;
>
> /* Create hash table for local ifunc. */
> ret->loc_hash_table = htab_try_create (1024,
> @@ -4460,17 +4464,27 @@ _bfd_riscv_relax_call (bfd *abfd, asection *sec,
> asection *sym_sec,
> link_info, pcgp_relocs, rel + 1);
> }
>
> -/* Traverse all output sections and return the max alignment. */
> +/* Traverse all output sections and return the max alignment.
> +
> + If gp is zero, then all the output section alignments are
> + possible candidates; Otherwise, only the output sections
> + which are in the [gp-2K, gp+2K) range need to be considered. */
>
> static bfd_vma
> -_bfd_riscv_get_max_alignment (asection *sec)
> +_bfd_riscv_get_max_alignment (asection *sec, bfd_vma gp)
> {
> unsigned int max_alignment_power = 0;
> asection *o;
>
> for (o = sec->output_section->owner->sections; o != NULL; o = o->next)
> {
> - if (o->alignment_power > max_alignment_power)
> + bool valid = true;
> + if (gp
> + && !(VALID_ITYPE_IMM (sec_addr (o) - gp)
> + || VALID_ITYPE_IMM (sec_addr (o) + o->size - gp)))
> + valid = false;
> +
> + if (valid && o->alignment_power > max_alignment_power)
> max_alignment_power = o->alignment_power;
> }
>
> @@ -4492,15 +4506,16 @@ _bfd_riscv_relax_lui (bfd *abfd,
> riscv_pcgp_relocs *pcgp_relocs,
> bool undefined_weak)
> {
> + struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table
> (link_info);
> bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
> - bfd_vma gp = riscv_elf_hash_table (link_info)->params->relax_gp
> - ? riscv_global_pointer_value (link_info)
> - : 0;
> + bfd_vma gp = htab->params->relax_gp
> + ? riscv_global_pointer_value (link_info)
> + : 0;
> int use_rvc = elf_elfheader (abfd)->e_flags & EF_RISCV_RVC;
>
> BFD_ASSERT (rel->r_offset + 4 <= sec->size);
>
> - if (gp)
> + if (!undefined_weak && gp)
> {
> /* If gp and the symbol are in the same output section, which is
> not the
> abs section, then consider only that output section's alignment.
> */
> @@ -4510,16 +4525,28 @@ _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
> + {
> + /* Consider output section alignments which are in [gp-2K,
> gp+2K). */
> + max_alignment = htab->max_alignment_for_gp;
> + if (max_alignment == (bfd_vma) -1)
> + {
> + max_alignment = _bfd_riscv_get_max_alignment (sec, gp);
> + htab->max_alignment_for_gp = max_alignment;
> + }
> + }
> }
>
> /* Is the reference in range of x0 or gp?
> - Valid gp range conservatively because of alignment issue. */
> + Valid gp range conservatively because of alignment issue.
> +
> + Should we also consider the alignment issue for x0 base? */
> if (undefined_weak
> - || (VALID_ITYPE_IMM (symval)
> - || (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)
> + || (symval >= gp
> + && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
> + || (symval < gp
> + && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
> {
> unsigned sym = ELFNN_R_SYM (rel->r_info);
> switch (ELFNN_R_TYPE (rel->r_info))
> @@ -4709,6 +4736,7 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
> riscv_pcgp_relocs *pcgp_relocs,
> bool undefined_weak)
> {
> + struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table
> (link_info);
> bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
> bfd_vma gp = riscv_global_pointer_value (link_info);
>
> @@ -4765,7 +4793,7 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
> abort ();
> }
>
> - if (gp)
> + if (!undefined_weak && gp)
> {
> /* If gp and the symbol are in the same output section, which is
> not the
> abs section, then consider only that output section's alignment.
> */
> @@ -4775,16 +4803,28 @@ _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
> + {
> + /* Consider output section alignments which are in [gp-2K,
> gp+2K). */
> + max_alignment = htab->max_alignment_for_gp;
> + if (max_alignment == (bfd_vma) -1)
> + {
> + max_alignment = _bfd_riscv_get_max_alignment (sec, gp);
> + htab->max_alignment_for_gp = max_alignment;
> + }
> + }
> }
>
> /* Is the reference in range of x0 or gp?
> - Valid gp range conservatively because of alignment issue. */
> + Valid gp range conservatively because of alignment issue.
> +
> + Should we also consider the alignment issue for x0 base? */
> if (undefined_weak
> - || (VALID_ITYPE_IMM (symval)
> - || (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)
> + || (symval >= gp
> + && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
> + || (symval < gp
> + && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
> {
> unsigned sym = hi_reloc.hi_sym;
> switch (ELFNN_R_TYPE (rel->r_info))
> @@ -4877,6 +4917,7 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
> unsigned int i;
> bfd_vma max_alignment, reserve_size = 0;
> riscv_pcgp_relocs pcgp_relocs;
> + static asection *first_section = NULL;
>
> *again = false;
>
> @@ -4892,6 +4933,13 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
> || *(htab->data_segment_phase) == 4)
> return true;
>
> + /* Record the first relax section, so that we can reset the
> + max_alignment_for_gp for the repeated relax passes. */
> + if (first_section == NULL)
> + first_section = sec;
> + else if (first_section == sec)
> + htab->max_alignment_for_gp = -1;
> +
> riscv_init_pcgp_relocs (&pcgp_relocs);
>
> /* Read this BFD's relocs if we haven't done so already. */
> @@ -4901,17 +4949,14 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
> info->keep_memory)))
> goto fail;
>
> - if (htab)
> + /* Estimate the maximum alignment for all output sections once time
> + should be enough. */
> + max_alignment = htab->max_alignment;
> + if (max_alignment == (bfd_vma) -1)
> {
> - max_alignment = htab->max_alignment;
> - if (max_alignment == (bfd_vma) -1)
> - {
> - max_alignment = _bfd_riscv_get_max_alignment (sec);
> - htab->max_alignment = max_alignment;
> - }
> + max_alignment = _bfd_riscv_get_max_alignment (sec, 0/* gp */);
> + htab->max_alignment = max_alignment;
> }
> - else
> - max_alignment = _bfd_riscv_get_max_alignment (sec);
>
> /* Examine and consider relaxing each reloc. */
> for (i = 0; i < sec->reloc_count; i++)
> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> index 43572c5286b..9e103b283f7 100644
> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> @@ -171,6 +171,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-gp"
> 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-gp.d
> b/ld/testsuite/ld-riscv-elf/relax-max-align-gp.d
> new file mode 100644
> index 00000000000..637de426ee4
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/relax-max-align-gp.d
> @@ -0,0 +1,46 @@
> +#source: relax-max-align-gp.s
> +#ld:
> +#objdump: -d
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+[0-9a-f]+ <_start>:
> +.*:[ ]+[0-9a-f]+[ ]+add[ ]+.*<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-gp.s
> b/ld/testsuite/ld-riscv-elf/relax-max-align-gp.s
> new file mode 100644
> index 00000000000..ce3da21e7f4
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/relax-max-align-gp.s
> @@ -0,0 +1,28 @@
> +
> +.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.39.2 (Apple Git-143)
>
>
prev parent reply other threads:[~2023-04-21 7:50 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-17 9:54 Nelson Chu
2023-04-21 7:50 ` Nelson Chu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPpQWtB=QNQX+FTPhScVJrK0LHUH8Vva3V+uEut=KFYeJXCOVA@mail.gmail.com' \
--to=nelson@rivosinc.com \
--cc=binutils@sourceware.org \
--cc=jim.wilson.gcc@gmail.com \
--cc=lifang_xia@linux.alibaba.com \
--cc=palmer@dabbelt.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).