From: "Xiao Zeng" <zengxiao@eswincomputing.com>
To: "Nelson Chu" <nelson@rivosinc.com>
Cc: binutils <binutils@sourceware.org>,
kito.cheng <kito.cheng@gmail.com>, palmer <palmer@dabbelt.com>,
zhengyu <zhengyu@eswincomputing.com>,
Jiawei <jiawei@iscas.ac.cn>
Subject: Re: Re: [PATCH] RISC-V: Avoid relocation for a function symbol self-referenced with -mno-relax
Date: Tue, 7 Jan 2025 11:03:52 +0800 [thread overview]
Message-ID: <202501071103521611936@eswincomputing.com> (raw)
In-Reply-To: <CAPpQWtCWyZ4gNCoKwNvy=5WHLuiw=Jd-FQ8vA8pNzW8kCVzCOA@mail.gmail.com>
2025-01-07 10:46 Nelson Chu <nelson@rivosinc.com> wrote:
>
>On Tue, Jan 7, 2025 at 9:54 AM Xiao Zeng <zengxiao@eswincomputing.com>
>wrote:
>
>> Refer to commit dff565fcca8137954d6ad571ef39f6aec5c0429c. When relaxation
>> is disabled, the assembler no longer needs to generate relocation
>> information for references to this symbol within its body. This commit
>> enhances such scenarios.
>>
>> gas/ChangeLog:
>>
>> * config/tc-riscv.c (md_apply_fix): Avoid relocation for a function
>> symbol self-referenced with -mno-relax.
>> * testsuite/gas/riscv/fixup-local-norelax.d: Updated.
>> * testsuite/gas/riscv/fixup-function-symbol-norelax.d: New test.
>> * testsuite/gas/riscv/fixup-function-symbol-relax.d: New test.
>> * testsuite/gas/riscv/fixup-function-symbol.s: New test.
>>
>> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com>
>> ---
>> gas/config/tc-riscv.c | 4 +-
>> .../gas/riscv/fixup-function-symbol-norelax.d | 26 ++++++++++
>> .../gas/riscv/fixup-function-symbol-relax.d | 50 +++++++++++++++++++
>> .../gas/riscv/fixup-function-symbol.s | 13 +++++
>> gas/testsuite/gas/riscv/fixup-local-norelax.d | 12 ++---
>> 5 files changed, 94 insertions(+), 11 deletions(-)
>> create mode 100644 gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> create mode 100644 gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> create mode 100644 gas/testsuite/gas/riscv/fixup-function-symbol.s
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index a915c8b4995..7255a2337f6 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -4800,7 +4800,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg)
>> Fill in a tentative value to improve objdump readability for
>> -mrelax,
>> and set fx_done for -mno-relax. */
>> if (fixP->fx_addsy
>> - && S_IS_LOCAL (fixP->fx_addsy)
>> + && (S_IS_LOCAL (fixP->fx_addsy) || !riscv_opts.relax)
>> && S_GET_SEGMENT (fixP->fx_addsy) == seg)
>> {
>> bfd_vma target = S_GET_VALUE (fixP->fx_addsy) + *valP;
>> @@ -4835,7 +4835,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg)
>> riscv_pcrel_hi_fixup *entry = htab_find (riscv_pcrel_hi_fixup_hash,
>> &search);
>> if (entry && entry->symbol
>> - && S_IS_LOCAL (entry->symbol)
>> + && (S_IS_LOCAL (entry->symbol) || !riscv_opts.relax)
>> && S_GET_SEGMENT (entry->symbol) == seg)
>> {
>> bfd_vma target = entry->target;
>>
>
>The original idea was to only optimize local symbols, since global/weak
>symbols may be preemptive in the link time, so we cannot resolve them in
>the assembler time.
Your point is correct, but considering this special case: When relaxation is
disabled, the assembler no longer needs to generate relocation information
for references to this symbol within its body. Regardless of whether it's a
global symbol, the relocation can be completed during the assembly phase,
without needing to defer it to the linking (ld) phase.
It is optimized specifically for this special scenario.
>
>
>> diff --git a/gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> b/gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> new file mode 100644
>> index 00000000000..5a69b1cc6fd
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> @@ -0,0 +1,26 @@
>> +#as: -march=rv64i -mno-relax
>> +#source: fixup-function-symbol.s
>> +#objdump: -dr -Mno-aliases
>> +
>> +.*:[ ]+file format .*
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +0+0000 <localFuncSym>:
>> +[ ]+0:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+4:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 0
>> <localFuncSym>
>> +[ ]+8:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+c:[ ]+ff853503[ ]+ld[ ]+a0,-8\(a0\) # 0 <localFuncSym>
>> +[ ]+10:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+14:[ ]+fea53823[ ]+sd[ ]+a0,-16\(a0\) # 0 <localFuncSym>
>> +[ ]+18:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> +
>> +0+001c <globalFuncSym>:
>> +[ ]+1c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+20:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 1c
>> <globalFuncSym>
>> +[ ]+24:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+28:[ ]+ff853503[ ]+ld[ ]+a0,-8\(a0\) # 1c <globalFuncSym>
>> +[ ]+2c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+30:[ ]+fea53823[ ]+sd[ ]+a0,-16\(a0\) # 1c <globalFuncSym>
>> +[ ]+34:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> \ No newline at end of file
>> diff --git a/gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> b/gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> new file mode 100644
>> index 00000000000..70381534720
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> @@ -0,0 +1,50 @@
>> +#as: -march=rv64i -mrelax
>> +#source: fixup-function-symbol.s
>> +#objdump: -dr -Mno-aliases
>> +
>> +.*:[ ]+file format .*
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +0+0000 <localFuncSym>:
>> +[ ]+0:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+0:[ ]+R_RISCV_PCREL_HI20[ ]+localFuncSym.*
>> +[ ]+0:[ ]+R_RISCV_RELAX.*
>> +[ ]+4:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 0
>> <localFuncSym>
>> +[ ]+4:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+4:[ ]+R_RISCV_RELAX.*
>> +[ ]+8:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+8:[ ]+R_RISCV_PCREL_HI20[ ]+localFuncSym.*
>> +[ ]+8:[ ]+R_RISCV_RELAX.*
>> +[ ]+c:[ ]+00053503[ ]+ld[ ]+a0,0\(a0\) # 8
>> <localFuncSym\+0x8>
>> +[ ]+c:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+c:[ ]+R_RISCV_RELAX.*
>> +[ ]+10:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+10:[ ]+R_RISCV_PCREL_HI20[ ]+localFuncSym.*
>> +[ ]+10:[ ]+R_RISCV_RELAX.*
>> +[ ]+14:[ ]+00a53023[ ]+sd[ ]+a0,0\(a0\) # 10
>> <localFuncSym\+0x10>
>> +[ ]+14:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> +[ ]+14:[ ]+R_RISCV_RELAX.*
>> +[ ]+18:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> +
>> +0+001c <globalFuncSym>:
>> +[ ]+1c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+1c:[ ]+R_RISCV_PCREL_HI20[ ]+globalFuncSym.*
>> +[ ]+1c:[ ]+R_RISCV_RELAX.*
>> +[ ]+20:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 1c
>> <globalFuncSym>
>> +[ ]+20:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+20:[ ]+R_RISCV_RELAX.*
>> +[ ]+24:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+24:[ ]+R_RISCV_PCREL_HI20[ ]+globalFuncSym.*
>> +[ ]+24:[ ]+R_RISCV_RELAX.*
>> +[ ]+28:[ ]+00053503[ ]+ld[ ]+a0,0\(a0\) # 24
>> <globalFuncSym\+0x8>
>> +[ ]+28:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+28:[ ]+R_RISCV_RELAX.*
>> +[ ]+2c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+2c:[ ]+R_RISCV_PCREL_HI20[ ]+globalFuncSym.*
>> +[ ]+2c:[ ]+R_RISCV_RELAX.*
>> +[ ]+30:[ ]+00a53023[ ]+sd[ ]+a0,0\(a0\) # 2c
>> <globalFuncSym\+0x10>
>> +[ ]+30:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> +[ ]+30:[ ]+R_RISCV_RELAX.*
>> +[ ]+34:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> \ No newline at end of file
>> diff --git a/gas/testsuite/gas/riscv/fixup-function-symbol.s
>> b/gas/testsuite/gas/riscv/fixup-function-symbol.s
>> new file mode 100644
>> index 00000000000..d93b29e17e7
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-function-symbol.s
>> @@ -0,0 +1,13 @@
>> + .local localFuncSym
>> +localFuncSym:
>> + la a0, localFuncSym
>> + ld a0, localFuncSym
>> + sd a0, localFuncSym, a0
>> + ret
>> +
>> + .global globalFuncSym
>> +globalFuncSym:
>> + la a0, globalFuncSym
>> + ld a0, globalFuncSym
>> + sd a0, globalFuncSym, a0
>> + ret
>>
>
>I don't know the difference between this testcase and fixup-local.s. Even
>so, we should merge them into one as possible as we can if they are trying
>to resolve the same issue...
Yes, there is indeed no significant difference between them. I just used more
informative code to express the intent of this patch, in order to avoid
breaking the previous use cases.
>
>Nelson
>
>
>> diff --git a/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> index 8b29860893c..bc0b6714265 100644
>> --- a/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> +++ b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> @@ -27,17 +27,11 @@ Disassembly of section .text:
>> [ ]+2c:[ ]+00a51023[ ]+sh[ ]+a0,0\(a0\) # 28 <foo\+0x28>
>> [ ]+2c:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> [ ]+30:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> -[ ]+30:[ ]+R_RISCV_PCREL_HI20[ ]+foo.*
>> -[ ]+34:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 30 <foo\+0x30>
>> -[ ]+34:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+34:[ ]+fd050513[ ]+addi[ ]+a0,a0,-48 # 0 <foo>
>
> [ ]+38:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> -[ ]+38:[ ]+R_RISCV_PCREL_HI20[ ]+foo.*
>> -[ ]+3c:[ ]+00051503[ ]+lh[ ]+a0,0\(a0\) # 38 <foo\+0x38>
>> -[ ]+3c:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+3c:[ ]+fc851503[ ]+lh[ ]+a0,-56\(a0\) # 0 <foo>
>> [ ]+40:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> -[ ]+40:[ ]+R_RISCV_PCREL_HI20[ ]+foo.*
>> -[ ]+44:[ ]+00a51023[ ]+sh[ ]+a0,0\(a0\) # 40 <foo\+0x40>
>> -[ ]+44:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> +[ ]+44:[ ]+fca51023[ ]+sh[ ]+a0,-64\(a0\) # 0 <foo>
>> [ ]+48:[ ]+00000517[ ]+auipc[ ]a0,0x0
>> [ ]+4c:[ ]+01052503[ ]+lw[ ]+a0,16\(a0\) # 58 <foo\+0x58>
>> [ ]+50:[ ]+00000517[ ]+auipc[ ]a0,0x0
>> --
>> 2.17.1
>>
>>
Of course, if Nelson approves this patch, I will reorganize the test cases as per
your suggestion and submit the v2 patch.
By the way, this patch has passed the regression tests in the riscv64 toolchain.
Thanks
Xiao Zeng
next prev parent reply other threads:[~2025-01-07 3:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-07 1:54 Xiao Zeng
2025-01-07 2:46 ` Nelson Chu
2025-01-07 3:03 ` Xiao Zeng [this message]
2025-01-07 4:02 ` Nelson Chu
2025-01-07 5:40 ` Xiao Zeng
2025-01-07 8:07 ` Jan Beulich
2025-01-07 8:33 ` Xiao Zeng
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=202501071103521611936@eswincomputing.com \
--to=zengxiao@eswincomputing.com \
--cc=binutils@sourceware.org \
--cc=jiawei@iscas.ac.cn \
--cc=kito.cheng@gmail.com \
--cc=nelson@rivosinc.com \
--cc=palmer@dabbelt.com \
--cc=zhengyu@eswincomputing.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).