From: Lifang Xia <lifang_xia@linux.alibaba.com>
To: Nelson Chu <nelson@rivosinc.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: Sat, 24 Sep 2022 09:54:44 +0800 [thread overview]
Message-ID: <eea67c16-bd5b-daf8-ad7d-997c9ab06183@linux.alibaba.com> (raw)
In-Reply-To: <CAPpQWtDZEo0uP4WAoDDXSLz2ren_uXFfqR9Mq2EOTmj=FfNQyw@mail.gmail.com>
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,
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)
>>
next prev parent reply other threads:[~2022-09-24 1:54 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 [this message]
2022-09-27 8:51 ` Nelson Chu
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=eea67c16-bd5b-daf8-ad7d-997c9ab06183@linux.alibaba.com \
--to=lifang_xia@linux.alibaba.com \
--cc=binutils@sourceware.org \
--cc=gkm@rivosinc.com \
--cc=nelson@rivosinc.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).