public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Nelson Chu <nelson@rivosinc.com>
To: Lifang Xia <lifang_xia@linux.alibaba.com>
Cc: binutils@sourceware.org, gkm@rivosinc.com,
	 Palmer Dabbelt <palmer@rivosinc.com>
Subject: Re: [PATCH] RISC-V: Optimize relax of GP with max_alignment.
Date: Tue, 27 Sep 2022 16:51:00 +0800	[thread overview]
Message-ID: <CAPpQWtBxNyqYjL-bsNCX4fUnXEknh6Ah_kd1oarGPGCZ-b=dgQ@mail.gmail.com> (raw)
In-Reply-To: <eea67c16-bd5b-daf8-ad7d-997c9ab06183@linux.alibaba.com>

On Sat, Sep 24, 2022 at 9:54 AM Lifang Xia <lifang_xia@linux.alibaba.com> wrote:
>
>
> On 2022/9/23 18:44, Nelson Chu wrote:
> > 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.
> Agree with that.
>
> >
> > 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.
>
> It would be occurence if the gp is assigned with a absolute address,
> such as "__global_point = 0x4000".
>
> >   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.
> That's is conservative idea, it would work fine for both of the test cases.
> > 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.
>
> It is not a good choice. Because the max alignment is only used for gp,
> but not all of the relax need it.

Thanks, make sense.  Then is it possible to estimate the max alignment
for gp only once in the _bfd_riscv_get_max_alignment_in_gp for each
relax_pass?

Nelson

> Thanks,
> Lifang
>
> >
> > 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)
> >>

  reply	other threads:[~2022-09-27  8:51 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-23  6:47 lifang_xia
2022-09-23  8:05 ` Nelson Chu
2022-09-23  9:09   ` Lifang Xia
2022-09-23  9:46 ` lifang_xia
2022-09-23 10:44   ` Nelson Chu
2022-09-24  1:54     ` Lifang Xia
2022-09-27  8:51       ` Nelson Chu [this message]
2022-10-27  3:19       ` [PATCH v2] RISC-V: Optimize relax of GP/call " lifang_xia
2022-11-09  2:52         ` Lifang Xia
2022-11-22 10:19         ` Nelson Chu
2022-11-22 16:19           ` Palmer Dabbelt

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='CAPpQWtBxNyqYjL-bsNCX4fUnXEknh6Ah_kd1oarGPGCZ-b=dgQ@mail.gmail.com' \
    --to=nelson@rivosinc.com \
    --cc=binutils@sourceware.org \
    --cc=gkm@rivosinc.com \
    --cc=lifang_xia@linux.alibaba.com \
    --cc=palmer@rivosinc.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).