* [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
@ 2023-11-29 9:17 lifang_xia
2023-12-12 9:46 ` Nelson Chu
0 siblings, 1 reply; 7+ messages in thread
From: lifang_xia @ 2023-11-29 9:17 UTC (permalink / raw)
To: binutils; +Cc: nelson, kito.cheng, andrew, palmer, Lifang Xia
From: Lifang Xia <lifang_xia@linux.alibaba.com>
In the scenario of generating .ko files, the kernel does not relax the .ko
files. However, due to the large amount of relax and local relocation
information, this increases the size of the .ko files.
In this patch, it will finish the fixup of the local relocations while with
"-mno-relax" option. This can reduce the size of the relocation table.
gas/
* config/tc-riscv.c (struct riscv_pcrel_hi_reloc): New, reference
from bfd/elfnn-riscv.c.
(riscv_pcrel_hi_reloc_hash): Likewise.
(riscv_pcrel_reloc_hash): Likewise.
(riscv_pcrel_reloc_eq): Likewise.
(riscv_record_pcrel_reolc): Likewise.
(md_begin): Init pcrel_hi hash.
(md_apply_fix) <case BFD_RELOC_RISCV_PCREL_HI20>: Do fixup and
record the pcrel_hi relocs, mark as done while with
"-mno-relax".
(md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_I>:
(md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_S>: Do fixup
and mark as done while with "-mno-relax".
(riscv_md_end): New, delete riscv_pcrel_hi_reloc_hash.
* config/tc-riscv.h (md_end): Define md_end with riscv_md_end.
gas/
* testsuite/gas/riscv/fixup-local*: New tests.
---
gas/config/tc-riscv.c | 111 +++++++++++++++++-
gas/config/tc-riscv.h | 3 +
gas/testsuite/gas/riscv/fixup-local-norelax.d | 23 ++++
gas/testsuite/gas/riscv/fixup-local-relax.d | 41 +++++++
gas/testsuite/gas/riscv/fixup-local.s | 13 ++
5 files changed, 190 insertions(+), 1 deletion(-)
create mode 100644 gas/testsuite/gas/riscv/fixup-local-norelax.d
create mode 100644 gas/testsuite/gas/riscv/fixup-local-relax.d
create mode 100644 gas/testsuite/gas/riscv/fixup-local.s
diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 04738d5e00c..332103574fe 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1569,6 +1569,58 @@ init_opcode_hash (const struct riscv_opcode *opcodes,
return hash;
}
+/* Record all PC-relative high-part relocs we've encountered to help us
+ later resolve the corresponding low-part relocs. */
+struct riscv_pcrel_hi_reloc
+{
+ bfd_vma address;
+ symbolS *symbol;
+ bfd_vma target;
+};
+
+/* Handle of the pcrel_hi hash table. */
+static htab_t riscv_pcrel_hi_reloc_hash;
+
+/* Get the key of a entry from the pcrel_hi hash table. */
+
+static hashval_t
+riscv_pcrel_reloc_hash (const void *entry)
+{
+ const struct riscv_pcrel_hi_reloc *e = entry;
+ return (hashval_t) (e->address);
+}
+
+/* Compare the keys between two entries fo the pcrel_hi hash table. */
+
+static int
+riscv_pcrel_reloc_eq (const void *entry1, const void *entry2)
+{
+ const struct riscv_pcrel_hi_reloc *e1 = entry1, *e2 = entry2;
+ return e1->address == e2->address;
+}
+
+/* Record the pcrel_hi relocs. */
+
+static bool
+riscv_record_pcrel_reolc (htab_t p, bfd_vma address, symbolS *symbol,
+ bfd_vma target)
+{
+ struct riscv_pcrel_hi_reloc entry = { address, symbol, target };
+ struct riscv_pcrel_hi_reloc **slot
+ = (struct riscv_pcrel_hi_reloc **)htab_find_slot (p, &entry, INSERT);
+ if (slot == NULL)
+ return false;
+
+ *slot = (struct riscv_pcrel_hi_reloc *)xmalloc (
+ sizeof (struct riscv_pcrel_hi_reloc));
+ if (*slot != NULL)
+ {
+ **slot = entry;
+ return true;
+ }
+ return false;
+}
+
/* This function is called once, at assembler startup time. It should set up
all the tables, etc. that the MD part of the assembler will need. */
@@ -1605,6 +1657,11 @@ md_begin (void)
opcode_names_hash = str_htab_create ();
init_opcode_names_hash ();
+ /* Create pcrel_hi hash table to resolve the relocation while with
+ -mno-relax. */
+ riscv_pcrel_hi_reloc_hash
+ = htab_create (1024, riscv_pcrel_reloc_hash, riscv_pcrel_reloc_eq, free);
+
/* Set the default alignment for the text section. */
record_alignment (text_section, riscv_opts.rvc ? 1 : 2);
}
@@ -4213,9 +4270,52 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
break;
case BFD_RELOC_RISCV_PCREL_HI20:
+ /* record the pcrel_hi relocs of the local symbols. And evaluate the hi20
+ of the lcoal symbols. Fill in a tentative value to improve objdump
+ readability for -mrelax, and set fx_done for -mno-relax. */
+ if (S_IS_LOCAL (fixP->fx_addsy) && fixP->fx_addsy
+ && S_GET_SEGMENT (fixP->fx_addsy) == seg)
+ {
+ bfd_vma target = S_GET_VALUE (fixP->fx_addsy) + *valP;
+ bfd_vma value = target - md_pcrel_from (fixP);
+
+ /* Record PCREL_HI20. */
+ if (!riscv_record_pcrel_reolc (riscv_pcrel_hi_reloc_hash,
+ md_pcrel_from (fixP), fixP->fx_addsy,
+ target))
+ {
+ as_warn ("too many pcrel_hi");
+ }
+
+ bfd_putl32 (bfd_getl32 (buf)
+ | ENCODE_UTYPE_IMM (RISCV_CONST_HIGH_PART (value)),
+ buf);
+ if (!riscv_opts.relax)
+ fixP->fx_done = 1;
+ }
+ relaxable = riscv_opts.relax;
+ break;
case BFD_RELOC_RISCV_PCREL_LO12_S:
case BFD_RELOC_RISCV_PCREL_LO12_I:
- relaxable = riscv_opts.relax;
+ {
+ /* Resolve the low12 of the local symboles with the pcrel_hi relocs.
+ Fill in a tentative value to improve objdump readability for
+ -mrelax, and set fx_done for -mno-relax. */
+ bfd_vma location_pcrel_hi = S_GET_VALUE (fixP->fx_addsy) + *valP;
+ struct riscv_pcrel_hi_reloc search = { location_pcrel_hi, 0, 0 };
+ struct riscv_pcrel_hi_reloc *entry
+ = htab_find (riscv_pcrel_hi_reloc_hash, &search);
+ if (entry && entry->symbol && S_IS_LOCAL (entry->symbol)
+ && S_GET_SEGMENT (entry->symbol) == seg)
+ {
+ bfd_vma target = entry->target;
+ bfd_vma value = target - entry->address;
+ bfd_putl32 (bfd_getl32 (buf) | ENCODE_ITYPE_IMM (value), buf);
+ if (!riscv_opts.relax)
+ fixP->fx_done = 1;
+ }
+ relaxable = riscv_opts.relax;
+ }
break;
case BFD_RELOC_RISCV_ALIGN:
@@ -4980,6 +5080,15 @@ riscv_md_finish (void)
riscv_set_public_attributes ();
if (riscv_opts.relax)
bfd_map_over_sections (stdoutput, riscv_insert_uleb128_fixes, NULL);
+
+}
+
+/* Called just before the assembler exits. */
+
+void
+riscv_md_end (void)
+{
+ htab_delete (riscv_pcrel_hi_reloc_hash);
}
/* Adjust the symbol table. */
diff --git a/gas/config/tc-riscv.h b/gas/config/tc-riscv.h
index 0c70c7d4739..9d2f05a4241 100644
--- a/gas/config/tc-riscv.h
+++ b/gas/config/tc-riscv.h
@@ -80,6 +80,9 @@ extern int riscv_parse_long_option (const char *);
extern void riscv_pre_output_hook (void);
#define GAS_SORT_RELOCS 1
+#define md_end riscv_md_end
+extern void riscv_md_end (void);
+
/* Let the linker resolve all the relocs due to relaxation. */
#define tc_fix_adjustable(fixp) 0
#define md_allow_local_subtract(l,r,s) 0
diff --git a/gas/testsuite/gas/riscv/fixup-local-norelax.d b/gas/testsuite/gas/riscv/fixup-local-norelax.d
new file mode 100644
index 00000000000..22132a32f91
--- /dev/null
+++ b/gas/testsuite/gas/riscv/fixup-local-norelax.d
@@ -0,0 +1,23 @@
+#as: -march=rv64i -mno-relax
+#source: fixup-local.s
+#objdump: -dr
+
+tmpdir/fixup-local.o: file format elf64-littleriscv
+
+
+Disassembly of section .text:
+
+0+0000 <foo>:
+[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <foo\+0x8>
+[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+8: R_RISCV_PCREL_HI20 bar
+[ ]+c:[ ]+00050513[ ]+mv a0,a0
+[ ]+c: R_RISCV_PCREL_LO12_I .L0
+[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+10: R_RISCV_PCREL_HI20 foo
+[ ]+14:[ ]+00050513[ ]+mv a0,a0
+[ ]+14: R_RISCV_PCREL_LO12_I .L0
+[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <foo\+0x20>
+[ ]+20:[ ]+00008067[ ]+ret
diff --git a/gas/testsuite/gas/riscv/fixup-local-relax.d b/gas/testsuite/gas/riscv/fixup-local-relax.d
new file mode 100644
index 00000000000..49c475c5dcb
--- /dev/null
+++ b/gas/testsuite/gas/riscv/fixup-local-relax.d
@@ -0,0 +1,41 @@
+#as: -march=rv64i
+#source: fixup-local.s
+#objdump: -dr
+
+tmpdir/fixup-local.o: file format elf64-littleriscv
+
+
+Disassembly of section .text:
+
+0+0000 <foo>:
+[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+0: R_RISCV_PCREL_HI20 .LL0
+[ ]+0: R_RISCV_RELAX \*ABS\*
+[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <.LL0>
+[ ]+4: R_RISCV_PCREL_LO12_I .L0
+[ ]+4: R_RISCV_RELAX \*ABS\*
+
+0000000000000008 <.LL0>:
+[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+8: R_RISCV_PCREL_HI20 bar
+[ ]+8: R_RISCV_RELAX \*ABS\*
+[ ]+c:[ ]+00050513[ ]+mv a0,a0
+[ ]+c: R_RISCV_PCREL_LO12_I .L0
+[ ]+c: R_RISCV_RELAX \*ABS\*
+[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+10: R_RISCV_PCREL_HI20 foo
+[ ]+10: R_RISCV_RELAX \*ABS\*
+[ ]+14:[ ]+00050513[ ]+mv a0,a0
+[ ]+14: R_RISCV_PCREL_LO12_I .L0
+[ ]+14: R_RISCV_RELAX \*ABS\*
+
+0000000000000018 <.LL1>:
+[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
+[ ]+18: R_RISCV_PCREL_HI20 .LL2
+[ ]+18: R_RISCV_RELAX \*ABS\*
+[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <.LL2>
+[ ]+1c: R_RISCV_PCREL_LO12_I .LL1
+[ ]+1c: R_RISCV_RELAX \*ABS\*
+
+0000000000000020 <.LL2>:
+[ ]+20:[ ]+00008067[ ]+ret
diff --git a/gas/testsuite/gas/riscv/fixup-local.s b/gas/testsuite/gas/riscv/fixup-local.s
new file mode 100644
index 00000000000..44b47311235
--- /dev/null
+++ b/gas/testsuite/gas/riscv/fixup-local.s
@@ -0,0 +1,13 @@
+.global foo
+.global bar
+foo:
+ la a0, .LL0
+.LL0:
+ la a0, bar
+ la a0, foo
+.LL1:
+ auipc a0, %pcrel_hi(.LL2)
+ lw a0, %pcrel_lo(.LL1)(a0)
+
+.LL2:
+ ret
--
2.39.2 (Apple Git-143)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
2023-11-29 9:17 [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax" lifang_xia
@ 2023-12-12 9:46 ` Nelson Chu
2023-12-12 10:44 ` Nelson Chu
0 siblings, 1 reply; 7+ messages in thread
From: Nelson Chu @ 2023-12-12 9:46 UTC (permalink / raw)
To: lifang_xia; +Cc: binutils, kito.cheng, andrew, palmer
[-- Attachment #1: Type: text/plain, Size: 12181 bytes --]
The idea looks good to me. Passed regressions of riscv-gnu-toolchain, so
committed with some minor changes and indent fixes.
There are some TODOs, but no rush to do for now,
1. The implementation is based on the code from bfd/elfnn-riscv.c. We
probably can move the code to bfd/elfxx-riscv.c, so that can reduce
duplicate code, just like what we did for the architecture parser. Before
that, I renamed functions and variables from *reloc* to *fixup*, to
distinguish the code from bfd/elfnn-riscv.c, since they are still a little
bit different.
2. Maybe not only pcrel_hi/lo12 relocation with local symbols can be resolved
at assembler time. Other pc-relative relocation, like branch, may also be
able to perform related optimizations.
Thanks
Nelson
On Wed, Nov 29, 2023 at 5:17 PM <lifang_xia@linux.alibaba.com> wrote:
> From: Lifang Xia <lifang_xia@linux.alibaba.com>
>
> In the scenario of generating .ko files, the kernel does not relax the .ko
> files. However, due to the large amount of relax and local relocation
> information, this increases the size of the .ko files.
>
> In this patch, it will finish the fixup of the local relocations while with
> "-mno-relax" option. This can reduce the size of the relocation table.
>
> gas/
> * config/tc-riscv.c (struct riscv_pcrel_hi_reloc): New, reference
> from bfd/elfnn-riscv.c.
> (riscv_pcrel_hi_reloc_hash): Likewise.
> (riscv_pcrel_reloc_hash): Likewise.
> (riscv_pcrel_reloc_eq): Likewise.
> (riscv_record_pcrel_reolc): Likewise.
> (md_begin): Init pcrel_hi hash.
> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_HI20>: Do fixup and
> record the pcrel_hi relocs, mark as done while with
> "-mno-relax".
> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_I>:
> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_S>: Do fixup
> and mark as done while with "-mno-relax".
> (riscv_md_end): New, delete riscv_pcrel_hi_reloc_hash.
> * config/tc-riscv.h (md_end): Define md_end with riscv_md_end.
> gas/
> * testsuite/gas/riscv/fixup-local*: New tests.
> ---
> gas/config/tc-riscv.c | 111 +++++++++++++++++-
> gas/config/tc-riscv.h | 3 +
> gas/testsuite/gas/riscv/fixup-local-norelax.d | 23 ++++
> gas/testsuite/gas/riscv/fixup-local-relax.d | 41 +++++++
> gas/testsuite/gas/riscv/fixup-local.s | 13 ++
> 5 files changed, 190 insertions(+), 1 deletion(-)
> create mode 100644 gas/testsuite/gas/riscv/fixup-local-norelax.d
> create mode 100644 gas/testsuite/gas/riscv/fixup-local-relax.d
> create mode 100644 gas/testsuite/gas/riscv/fixup-local.s
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 04738d5e00c..332103574fe 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1569,6 +1569,58 @@ init_opcode_hash (const struct riscv_opcode
> *opcodes,
> return hash;
> }
>
> +/* Record all PC-relative high-part relocs we've encountered to help us
> + later resolve the corresponding low-part relocs. */
> +struct riscv_pcrel_hi_reloc
> +{
> + bfd_vma address;
> + symbolS *symbol;
> + bfd_vma target;
> +};
> +
> +/* Handle of the pcrel_hi hash table. */
> +static htab_t riscv_pcrel_hi_reloc_hash;
> +
> +/* Get the key of a entry from the pcrel_hi hash table. */
> +
> +static hashval_t
> +riscv_pcrel_reloc_hash (const void *entry)
> +{
> + const struct riscv_pcrel_hi_reloc *e = entry;
> + return (hashval_t) (e->address);
> +}
> +
> +/* Compare the keys between two entries fo the pcrel_hi hash table. */
> +
> +static int
> +riscv_pcrel_reloc_eq (const void *entry1, const void *entry2)
> +{
> + const struct riscv_pcrel_hi_reloc *e1 = entry1, *e2 = entry2;
> + return e1->address == e2->address;
> +}
> +
> +/* Record the pcrel_hi relocs. */
> +
> +static bool
> +riscv_record_pcrel_reolc (htab_t p, bfd_vma address, symbolS *symbol,
> + bfd_vma target)
> +{
> + struct riscv_pcrel_hi_reloc entry = { address, symbol, target };
> + struct riscv_pcrel_hi_reloc **slot
> + = (struct riscv_pcrel_hi_reloc **)htab_find_slot (p, &entry,
> INSERT);
> + if (slot == NULL)
> + return false;
> +
> + *slot = (struct riscv_pcrel_hi_reloc *)xmalloc (
> + sizeof (struct riscv_pcrel_hi_reloc));
> + if (*slot != NULL)
> + {
> + **slot = entry;
> + return true;
> + }
> + return false;
> +}
> +
> /* This function is called once, at assembler startup time. It should
> set up
> all the tables, etc. that the MD part of the assembler will need. */
>
> @@ -1605,6 +1657,11 @@ md_begin (void)
> opcode_names_hash = str_htab_create ();
> init_opcode_names_hash ();
>
> + /* Create pcrel_hi hash table to resolve the relocation while with
> + -mno-relax. */
> + riscv_pcrel_hi_reloc_hash
> + = htab_create (1024, riscv_pcrel_reloc_hash, riscv_pcrel_reloc_eq,
> free);
> +
> /* Set the default alignment for the text section. */
> record_alignment (text_section, riscv_opts.rvc ? 1 : 2);
> }
> @@ -4213,9 +4270,52 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg
> ATTRIBUTE_UNUSED)
> break;
>
> case BFD_RELOC_RISCV_PCREL_HI20:
> + /* record the pcrel_hi relocs of the local symbols. And evaluate
> the hi20
> + of the lcoal symbols. Fill in a tentative value to improve
> objdump
> + readability for -mrelax, and set fx_done for -mno-relax. */
> + if (S_IS_LOCAL (fixP->fx_addsy) && fixP->fx_addsy
> + && S_GET_SEGMENT (fixP->fx_addsy) == seg)
> + {
> + bfd_vma target = S_GET_VALUE (fixP->fx_addsy) + *valP;
> + bfd_vma value = target - md_pcrel_from (fixP);
> +
> + /* Record PCREL_HI20. */
> + if (!riscv_record_pcrel_reolc (riscv_pcrel_hi_reloc_hash,
> + md_pcrel_from (fixP),
> fixP->fx_addsy,
> + target))
> + {
> + as_warn ("too many pcrel_hi");
> + }
> +
> + bfd_putl32 (bfd_getl32 (buf)
> + | ENCODE_UTYPE_IMM (RISCV_CONST_HIGH_PART
> (value)),
> + buf);
> + if (!riscv_opts.relax)
> + fixP->fx_done = 1;
> + }
> + relaxable = riscv_opts.relax;
> + break;
> case BFD_RELOC_RISCV_PCREL_LO12_S:
> case BFD_RELOC_RISCV_PCREL_LO12_I:
> - relaxable = riscv_opts.relax;
> + {
> + /* Resolve the low12 of the local symboles with the pcrel_hi
> relocs.
> + Fill in a tentative value to improve objdump readability for
> + -mrelax, and set fx_done for -mno-relax. */
> + bfd_vma location_pcrel_hi = S_GET_VALUE (fixP->fx_addsy) + *valP;
> + struct riscv_pcrel_hi_reloc search = { location_pcrel_hi, 0, 0 };
> + struct riscv_pcrel_hi_reloc *entry
> + = htab_find (riscv_pcrel_hi_reloc_hash, &search);
> + if (entry && entry->symbol && S_IS_LOCAL (entry->symbol)
> + && S_GET_SEGMENT (entry->symbol) == seg)
> + {
> + bfd_vma target = entry->target;
> + bfd_vma value = target - entry->address;
> + bfd_putl32 (bfd_getl32 (buf) | ENCODE_ITYPE_IMM (value), buf);
> + if (!riscv_opts.relax)
> + fixP->fx_done = 1;
> + }
> + relaxable = riscv_opts.relax;
> + }
> break;
>
> case BFD_RELOC_RISCV_ALIGN:
> @@ -4980,6 +5080,15 @@ riscv_md_finish (void)
> riscv_set_public_attributes ();
> if (riscv_opts.relax)
> bfd_map_over_sections (stdoutput, riscv_insert_uleb128_fixes, NULL);
> +
> +}
> +
> +/* Called just before the assembler exits. */
> +
> +void
> +riscv_md_end (void)
> +{
> + htab_delete (riscv_pcrel_hi_reloc_hash);
> }
>
> /* Adjust the symbol table. */
> diff --git a/gas/config/tc-riscv.h b/gas/config/tc-riscv.h
> index 0c70c7d4739..9d2f05a4241 100644
> --- a/gas/config/tc-riscv.h
> +++ b/gas/config/tc-riscv.h
> @@ -80,6 +80,9 @@ extern int riscv_parse_long_option (const char *);
> extern void riscv_pre_output_hook (void);
> #define GAS_SORT_RELOCS 1
>
> +#define md_end riscv_md_end
> +extern void riscv_md_end (void);
> +
> /* Let the linker resolve all the relocs due to relaxation. */
> #define tc_fix_adjustable(fixp) 0
> #define md_allow_local_subtract(l,r,s) 0
> diff --git a/gas/testsuite/gas/riscv/fixup-local-norelax.d
> b/gas/testsuite/gas/riscv/fixup-local-norelax.d
> new file mode 100644
> index 00000000000..22132a32f91
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/fixup-local-norelax.d
> @@ -0,0 +1,23 @@
> +#as: -march=rv64i -mno-relax
> +#source: fixup-local.s
> +#objdump: -dr
> +
> +tmpdir/fixup-local.o: file format elf64-littleriscv
> +
> +
> +Disassembly of section .text:
> +
> +0+0000 <foo>:
> +[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <foo\+0x8>
> +[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+8: R_RISCV_PCREL_HI20 bar
> +[ ]+c:[ ]+00050513[ ]+mv a0,a0
> +[ ]+c: R_RISCV_PCREL_LO12_I .L0
> +[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+10: R_RISCV_PCREL_HI20 foo
> +[ ]+14:[ ]+00050513[ ]+mv a0,a0
> +[ ]+14: R_RISCV_PCREL_LO12_I .L0
> +[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <foo\+0x20>
> +[ ]+20:[ ]+00008067[ ]+ret
> diff --git a/gas/testsuite/gas/riscv/fixup-local-relax.d
> b/gas/testsuite/gas/riscv/fixup-local-relax.d
> new file mode 100644
> index 00000000000..49c475c5dcb
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/fixup-local-relax.d
> @@ -0,0 +1,41 @@
> +#as: -march=rv64i
> +#source: fixup-local.s
> +#objdump: -dr
> +
> +tmpdir/fixup-local.o: file format elf64-littleriscv
> +
> +
> +Disassembly of section .text:
> +
> +0+0000 <foo>:
> +[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+0: R_RISCV_PCREL_HI20 .LL0
> +[ ]+0: R_RISCV_RELAX \*ABS\*
> +[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <.LL0>
> +[ ]+4: R_RISCV_PCREL_LO12_I .L0
> +[ ]+4: R_RISCV_RELAX \*ABS\*
> +
> +0000000000000008 <.LL0>:
> +[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+8: R_RISCV_PCREL_HI20 bar
> +[ ]+8: R_RISCV_RELAX \*ABS\*
> +[ ]+c:[ ]+00050513[ ]+mv a0,a0
> +[ ]+c: R_RISCV_PCREL_LO12_I .L0
> +[ ]+c: R_RISCV_RELAX \*ABS\*
> +[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+10: R_RISCV_PCREL_HI20 foo
> +[ ]+10: R_RISCV_RELAX \*ABS\*
> +[ ]+14:[ ]+00050513[ ]+mv a0,a0
> +[ ]+14: R_RISCV_PCREL_LO12_I .L0
> +[ ]+14: R_RISCV_RELAX \*ABS\*
> +
> +0000000000000018 <.LL1>:
> +[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
> +[ ]+18: R_RISCV_PCREL_HI20 .LL2
> +[ ]+18: R_RISCV_RELAX \*ABS\*
> +[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <.LL2>
> +[ ]+1c: R_RISCV_PCREL_LO12_I .LL1
> +[ ]+1c: R_RISCV_RELAX \*ABS\*
> +
> +0000000000000020 <.LL2>:
> +[ ]+20:[ ]+00008067[ ]+ret
> diff --git a/gas/testsuite/gas/riscv/fixup-local.s
> b/gas/testsuite/gas/riscv/fixup-local.s
> new file mode 100644
> index 00000000000..44b47311235
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/fixup-local.s
> @@ -0,0 +1,13 @@
> +.global foo
> +.global bar
> +foo:
> + la a0, .LL0
> +.LL0:
> + la a0, bar
> + la a0, foo
> +.LL1:
> + auipc a0, %pcrel_hi(.LL2)
> + lw a0, %pcrel_lo(.LL1)(a0)
> +
> +.LL2:
> + ret
> --
> 2.39.2 (Apple Git-143)
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
2023-12-12 9:46 ` Nelson Chu
@ 2023-12-12 10:44 ` Nelson Chu
2023-12-13 0:31 ` Palmer Dabbelt
0 siblings, 1 reply; 7+ messages in thread
From: Nelson Chu @ 2023-12-12 10:44 UTC (permalink / raw)
To: lifang_xia; +Cc: binutils, kito.cheng, andrew, palmer
[-- Attachment #1: Type: text/plain, Size: 13211 bytes --]
There is another stuff, do we need to limit linkers that only can link the
objects with -mno-relax if this optimization applied? If so, then we
probably need,
1. An assembler option for this optimization
2. Record the object is relaxable or not into the elf attribute? or
readelf header? or ...
2.1 Maybe we can record the relaxation information with
Tag_RISCV_x3_reg_usage?,
https://sourceware.org/pipermail/binutils/2023-September/129500.html?
2.2 Once the object enables relaxation for some code, the object needs to
be marked as "relaxable", even if it sets `.option norelax' later.
On Tue, Dec 12, 2023 at 5:46 PM Nelson Chu <nelson@rivosinc.com> wrote:
> The idea looks good to me. Passed regressions of riscv-gnu-toolchain, so
> committed with some minor changes and indent fixes.
>
> There are some TODOs, but no rush to do for now,
> 1. The implementation is based on the code from bfd/elfnn-riscv.c. We
> probably can move the code to bfd/elfxx-riscv.c, so that can reduce
> duplicate code, just like what we did for the architecture parser.
> Before that, I renamed functions and variables from *reloc* to *fixup*, to
> distinguish the code from bfd/elfnn-riscv.c, since they are still a little
> bit different.
> 2. Maybe not only pcrel_hi/lo12 relocation with local symbols can be resolved
> at assembler time. Other pc-relative relocation, like branch, may also
> be able to perform related optimizations.
>
> Thanks
> Nelson
>
> On Wed, Nov 29, 2023 at 5:17 PM <lifang_xia@linux.alibaba.com> wrote:
>
>> From: Lifang Xia <lifang_xia@linux.alibaba.com>
>>
>> In the scenario of generating .ko files, the kernel does not relax the .ko
>> files. However, due to the large amount of relax and local relocation
>> information, this increases the size of the .ko files.
>>
>> In this patch, it will finish the fixup of the local relocations while
>> with
>> "-mno-relax" option. This can reduce the size of the relocation table.
>>
>> gas/
>> * config/tc-riscv.c (struct riscv_pcrel_hi_reloc): New, reference
>> from bfd/elfnn-riscv.c.
>> (riscv_pcrel_hi_reloc_hash): Likewise.
>> (riscv_pcrel_reloc_hash): Likewise.
>> (riscv_pcrel_reloc_eq): Likewise.
>> (riscv_record_pcrel_reolc): Likewise.
>> (md_begin): Init pcrel_hi hash.
>> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_HI20>: Do fixup and
>> record the pcrel_hi relocs, mark as done while with
>> "-mno-relax".
>> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_I>:
>> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_S>: Do fixup
>> and mark as done while with "-mno-relax".
>> (riscv_md_end): New, delete riscv_pcrel_hi_reloc_hash.
>> * config/tc-riscv.h (md_end): Define md_end with riscv_md_end.
>> gas/
>> * testsuite/gas/riscv/fixup-local*: New tests.
>> ---
>> gas/config/tc-riscv.c | 111 +++++++++++++++++-
>> gas/config/tc-riscv.h | 3 +
>> gas/testsuite/gas/riscv/fixup-local-norelax.d | 23 ++++
>> gas/testsuite/gas/riscv/fixup-local-relax.d | 41 +++++++
>> gas/testsuite/gas/riscv/fixup-local.s | 13 ++
>> 5 files changed, 190 insertions(+), 1 deletion(-)
>> create mode 100644 gas/testsuite/gas/riscv/fixup-local-norelax.d
>> create mode 100644 gas/testsuite/gas/riscv/fixup-local-relax.d
>> create mode 100644 gas/testsuite/gas/riscv/fixup-local.s
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index 04738d5e00c..332103574fe 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1569,6 +1569,58 @@ init_opcode_hash (const struct riscv_opcode
>> *opcodes,
>> return hash;
>> }
>>
>> +/* Record all PC-relative high-part relocs we've encountered to help us
>> + later resolve the corresponding low-part relocs. */
>> +struct riscv_pcrel_hi_reloc
>> +{
>> + bfd_vma address;
>> + symbolS *symbol;
>> + bfd_vma target;
>> +};
>> +
>> +/* Handle of the pcrel_hi hash table. */
>> +static htab_t riscv_pcrel_hi_reloc_hash;
>> +
>> +/* Get the key of a entry from the pcrel_hi hash table. */
>> +
>> +static hashval_t
>> +riscv_pcrel_reloc_hash (const void *entry)
>> +{
>> + const struct riscv_pcrel_hi_reloc *e = entry;
>> + return (hashval_t) (e->address);
>> +}
>> +
>> +/* Compare the keys between two entries fo the pcrel_hi hash table. */
>> +
>> +static int
>> +riscv_pcrel_reloc_eq (const void *entry1, const void *entry2)
>> +{
>> + const struct riscv_pcrel_hi_reloc *e1 = entry1, *e2 = entry2;
>> + return e1->address == e2->address;
>> +}
>> +
>> +/* Record the pcrel_hi relocs. */
>> +
>> +static bool
>> +riscv_record_pcrel_reolc (htab_t p, bfd_vma address, symbolS *symbol,
>> + bfd_vma target)
>> +{
>> + struct riscv_pcrel_hi_reloc entry = { address, symbol, target };
>> + struct riscv_pcrel_hi_reloc **slot
>> + = (struct riscv_pcrel_hi_reloc **)htab_find_slot (p, &entry,
>> INSERT);
>> + if (slot == NULL)
>> + return false;
>> +
>> + *slot = (struct riscv_pcrel_hi_reloc *)xmalloc (
>> + sizeof (struct riscv_pcrel_hi_reloc));
>> + if (*slot != NULL)
>> + {
>> + **slot = entry;
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> /* This function is called once, at assembler startup time. It should
>> set up
>> all the tables, etc. that the MD part of the assembler will need. */
>>
>> @@ -1605,6 +1657,11 @@ md_begin (void)
>> opcode_names_hash = str_htab_create ();
>> init_opcode_names_hash ();
>>
>> + /* Create pcrel_hi hash table to resolve the relocation while with
>> + -mno-relax. */
>> + riscv_pcrel_hi_reloc_hash
>> + = htab_create (1024, riscv_pcrel_reloc_hash, riscv_pcrel_reloc_eq,
>> free);
>> +
>> /* Set the default alignment for the text section. */
>> record_alignment (text_section, riscv_opts.rvc ? 1 : 2);
>> }
>> @@ -4213,9 +4270,52 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg
>> ATTRIBUTE_UNUSED)
>> break;
>>
>> case BFD_RELOC_RISCV_PCREL_HI20:
>> + /* record the pcrel_hi relocs of the local symbols. And evaluate
>> the hi20
>> + of the lcoal symbols. Fill in a tentative value to improve
>> objdump
>> + readability for -mrelax, and set fx_done for -mno-relax. */
>> + if (S_IS_LOCAL (fixP->fx_addsy) && fixP->fx_addsy
>> + && S_GET_SEGMENT (fixP->fx_addsy) == seg)
>> + {
>> + bfd_vma target = S_GET_VALUE (fixP->fx_addsy) + *valP;
>> + bfd_vma value = target - md_pcrel_from (fixP);
>> +
>> + /* Record PCREL_HI20. */
>> + if (!riscv_record_pcrel_reolc (riscv_pcrel_hi_reloc_hash,
>> + md_pcrel_from (fixP),
>> fixP->fx_addsy,
>> + target))
>> + {
>> + as_warn ("too many pcrel_hi");
>> + }
>> +
>> + bfd_putl32 (bfd_getl32 (buf)
>> + | ENCODE_UTYPE_IMM (RISCV_CONST_HIGH_PART
>> (value)),
>> + buf);
>> + if (!riscv_opts.relax)
>> + fixP->fx_done = 1;
>> + }
>> + relaxable = riscv_opts.relax;
>> + break;
>> case BFD_RELOC_RISCV_PCREL_LO12_S:
>> case BFD_RELOC_RISCV_PCREL_LO12_I:
>> - relaxable = riscv_opts.relax;
>> + {
>> + /* Resolve the low12 of the local symboles with the pcrel_hi
>> relocs.
>> + Fill in a tentative value to improve objdump readability for
>> + -mrelax, and set fx_done for -mno-relax. */
>> + bfd_vma location_pcrel_hi = S_GET_VALUE (fixP->fx_addsy) + *valP;
>> + struct riscv_pcrel_hi_reloc search = { location_pcrel_hi, 0, 0 };
>> + struct riscv_pcrel_hi_reloc *entry
>> + = htab_find (riscv_pcrel_hi_reloc_hash, &search);
>> + if (entry && entry->symbol && S_IS_LOCAL (entry->symbol)
>> + && S_GET_SEGMENT (entry->symbol) == seg)
>> + {
>> + bfd_vma target = entry->target;
>> + bfd_vma value = target - entry->address;
>> + bfd_putl32 (bfd_getl32 (buf) | ENCODE_ITYPE_IMM (value),
>> buf);
>> + if (!riscv_opts.relax)
>> + fixP->fx_done = 1;
>> + }
>> + relaxable = riscv_opts.relax;
>> + }
>> break;
>>
>> case BFD_RELOC_RISCV_ALIGN:
>> @@ -4980,6 +5080,15 @@ riscv_md_finish (void)
>> riscv_set_public_attributes ();
>> if (riscv_opts.relax)
>> bfd_map_over_sections (stdoutput, riscv_insert_uleb128_fixes, NULL);
>> +
>> +}
>> +
>> +/* Called just before the assembler exits. */
>> +
>> +void
>> +riscv_md_end (void)
>> +{
>> + htab_delete (riscv_pcrel_hi_reloc_hash);
>> }
>>
>> /* Adjust the symbol table. */
>> diff --git a/gas/config/tc-riscv.h b/gas/config/tc-riscv.h
>> index 0c70c7d4739..9d2f05a4241 100644
>> --- a/gas/config/tc-riscv.h
>> +++ b/gas/config/tc-riscv.h
>> @@ -80,6 +80,9 @@ extern int riscv_parse_long_option (const char *);
>> extern void riscv_pre_output_hook (void);
>> #define GAS_SORT_RELOCS 1
>>
>> +#define md_end riscv_md_end
>> +extern void riscv_md_end (void);
>> +
>> /* Let the linker resolve all the relocs due to relaxation. */
>> #define tc_fix_adjustable(fixp) 0
>> #define md_allow_local_subtract(l,r,s) 0
>> diff --git a/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> new file mode 100644
>> index 00000000000..22132a32f91
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> @@ -0,0 +1,23 @@
>> +#as: -march=rv64i -mno-relax
>> +#source: fixup-local.s
>> +#objdump: -dr
>> +
>> +tmpdir/fixup-local.o: file format elf64-littleriscv
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +0+0000 <foo>:
>> +[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <foo\+0x8>
>> +[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+8: R_RISCV_PCREL_HI20 bar
>> +[ ]+c:[ ]+00050513[ ]+mv a0,a0
>> +[ ]+c: R_RISCV_PCREL_LO12_I .L0
>> +[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+10: R_RISCV_PCREL_HI20 foo
>> +[ ]+14:[ ]+00050513[ ]+mv a0,a0
>> +[ ]+14: R_RISCV_PCREL_LO12_I .L0
>> +[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <foo\+0x20>
>> +[ ]+20:[ ]+00008067[ ]+ret
>> diff --git a/gas/testsuite/gas/riscv/fixup-local-relax.d
>> b/gas/testsuite/gas/riscv/fixup-local-relax.d
>> new file mode 100644
>> index 00000000000..49c475c5dcb
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-local-relax.d
>> @@ -0,0 +1,41 @@
>> +#as: -march=rv64i
>> +#source: fixup-local.s
>> +#objdump: -dr
>> +
>> +tmpdir/fixup-local.o: file format elf64-littleriscv
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +0+0000 <foo>:
>> +[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+0: R_RISCV_PCREL_HI20 .LL0
>> +[ ]+0: R_RISCV_RELAX \*ABS\*
>> +[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <.LL0>
>> +[ ]+4: R_RISCV_PCREL_LO12_I .L0
>> +[ ]+4: R_RISCV_RELAX \*ABS\*
>> +
>> +0000000000000008 <.LL0>:
>> +[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+8: R_RISCV_PCREL_HI20 bar
>> +[ ]+8: R_RISCV_RELAX \*ABS\*
>> +[ ]+c:[ ]+00050513[ ]+mv a0,a0
>> +[ ]+c: R_RISCV_PCREL_LO12_I .L0
>> +[ ]+c: R_RISCV_RELAX \*ABS\*
>> +[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+10: R_RISCV_PCREL_HI20 foo
>> +[ ]+10: R_RISCV_RELAX \*ABS\*
>> +[ ]+14:[ ]+00050513[ ]+mv a0,a0
>> +[ ]+14: R_RISCV_PCREL_LO12_I .L0
>> +[ ]+14: R_RISCV_RELAX \*ABS\*
>> +
>> +0000000000000018 <.LL1>:
>> +[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
>> +[ ]+18: R_RISCV_PCREL_HI20 .LL2
>> +[ ]+18: R_RISCV_RELAX \*ABS\*
>> +[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <.LL2>
>> +[ ]+1c: R_RISCV_PCREL_LO12_I .LL1
>> +[ ]+1c: R_RISCV_RELAX \*ABS\*
>> +
>> +0000000000000020 <.LL2>:
>> +[ ]+20:[ ]+00008067[ ]+ret
>> diff --git a/gas/testsuite/gas/riscv/fixup-local.s
>> b/gas/testsuite/gas/riscv/fixup-local.s
>> new file mode 100644
>> index 00000000000..44b47311235
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-local.s
>> @@ -0,0 +1,13 @@
>> +.global foo
>> +.global bar
>> +foo:
>> + la a0, .LL0
>> +.LL0:
>> + la a0, bar
>> + la a0, foo
>> +.LL1:
>> + auipc a0, %pcrel_hi(.LL2)
>> + lw a0, %pcrel_lo(.LL1)(a0)
>> +
>> +.LL2:
>> + ret
>> --
>> 2.39.2 (Apple Git-143)
>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
2023-12-12 10:44 ` Nelson Chu
@ 2023-12-13 0:31 ` Palmer Dabbelt
2023-12-13 1:26 ` Fangrui Song
0 siblings, 1 reply; 7+ messages in thread
From: Palmer Dabbelt @ 2023-12-13 0:31 UTC (permalink / raw)
To: nelson; +Cc: lifang_xia, binutils, Kito Cheng, Andrew Waterman
On Tue, 12 Dec 2023 02:44:07 PST (-0800), nelson@rivosinc.com wrote:
> There is another stuff, do we need to limit linkers that only can link the
> objects with -mno-relax if this optimization applied? If so, then we
> probably need,
> 1. An assembler option for this optimization
> 2. Record the object is relaxable or not into the elf attribute? or
> readelf header? or ...
> 2.1 Maybe we can record the relaxation information with
> Tag_RISCV_x3_reg_usage?,
> https://sourceware.org/pipermail/binutils/2023-September/129500.html?
> 2.2 Once the object enables relaxation for some code, the object needs to
> be marked as "relaxable", even if it sets `.option norelax' later.
>
> On Tue, Dec 12, 2023 at 5:46 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
>> The idea looks good to me. Passed regressions of riscv-gnu-toolchain, so
>> committed with some minor changes and indent fixes.
>>
>> There are some TODOs, but no rush to do for now,
>> 1. The implementation is based on the code from bfd/elfnn-riscv.c. We
>> probably can move the code to bfd/elfxx-riscv.c, so that can reduce
>> duplicate code, just like what we did for the architecture parser.
>> Before that, I renamed functions and variables from *reloc* to *fixup*, to
>> distinguish the code from bfd/elfnn-riscv.c, since they are still a little
>> bit different.
>> 2. Maybe not only pcrel_hi/lo12 relocation with local symbols can be resolved
>> at assembler time. Other pc-relative relocation, like branch, may also
>> be able to perform related optimizations.
Nelson and I were just talking about this. I'd been leaning towards
adding another option along the lines of "-mno-relax-abi", which would
explicitly mean that users can depend on no objects being relaxed. That
said, I'm not actually sure I can come up with a case where anything
breaks.
I was specifically worried about things like the compiler doing label
subtraction, but IIUC that can only happen within a single translation
unit so we're safe to take advantage of on R_RISCV_RELAX relocations.
There's also cases like misaligned globals, but we're already broken
there so I'm not sure it counts.
So maybe this safe?
>>
>> Thanks
>> Nelson
>>
>> On Wed, Nov 29, 2023 at 5:17 PM <lifang_xia@linux.alibaba.com> wrote:
>>
>>> From: Lifang Xia <lifang_xia@linux.alibaba.com>
>>>
>>> In the scenario of generating .ko files, the kernel does not relax the .ko
>>> files. However, due to the large amount of relax and local relocation
>>> information, this increases the size of the .ko files.
>>>
>>> In this patch, it will finish the fixup of the local relocations while
>>> with
>>> "-mno-relax" option. This can reduce the size of the relocation table.
>>>
>>> gas/
>>> * config/tc-riscv.c (struct riscv_pcrel_hi_reloc): New, reference
>>> from bfd/elfnn-riscv.c.
>>> (riscv_pcrel_hi_reloc_hash): Likewise.
>>> (riscv_pcrel_reloc_hash): Likewise.
>>> (riscv_pcrel_reloc_eq): Likewise.
>>> (riscv_record_pcrel_reolc): Likewise.
>>> (md_begin): Init pcrel_hi hash.
>>> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_HI20>: Do fixup and
>>> record the pcrel_hi relocs, mark as done while with
>>> "-mno-relax".
>>> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_I>:
>>> (md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_S>: Do fixup
>>> and mark as done while with "-mno-relax".
>>> (riscv_md_end): New, delete riscv_pcrel_hi_reloc_hash.
>>> * config/tc-riscv.h (md_end): Define md_end with riscv_md_end.
>>> gas/
>>> * testsuite/gas/riscv/fixup-local*: New tests.
>>> ---
>>> gas/config/tc-riscv.c | 111 +++++++++++++++++-
>>> gas/config/tc-riscv.h | 3 +
>>> gas/testsuite/gas/riscv/fixup-local-norelax.d | 23 ++++
>>> gas/testsuite/gas/riscv/fixup-local-relax.d | 41 +++++++
>>> gas/testsuite/gas/riscv/fixup-local.s | 13 ++
>>> 5 files changed, 190 insertions(+), 1 deletion(-)
>>> create mode 100644 gas/testsuite/gas/riscv/fixup-local-norelax.d
>>> create mode 100644 gas/testsuite/gas/riscv/fixup-local-relax.d
>>> create mode 100644 gas/testsuite/gas/riscv/fixup-local.s
>>>
>>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>>> index 04738d5e00c..332103574fe 100644
>>> --- a/gas/config/tc-riscv.c
>>> +++ b/gas/config/tc-riscv.c
>>> @@ -1569,6 +1569,58 @@ init_opcode_hash (const struct riscv_opcode
>>> *opcodes,
>>> return hash;
>>> }
>>>
>>> +/* Record all PC-relative high-part relocs we've encountered to help us
>>> + later resolve the corresponding low-part relocs. */
>>> +struct riscv_pcrel_hi_reloc
>>> +{
>>> + bfd_vma address;
>>> + symbolS *symbol;
>>> + bfd_vma target;
>>> +};
>>> +
>>> +/* Handle of the pcrel_hi hash table. */
>>> +static htab_t riscv_pcrel_hi_reloc_hash;
>>> +
>>> +/* Get the key of a entry from the pcrel_hi hash table. */
>>> +
>>> +static hashval_t
>>> +riscv_pcrel_reloc_hash (const void *entry)
>>> +{
>>> + const struct riscv_pcrel_hi_reloc *e = entry;
>>> + return (hashval_t) (e->address);
>>> +}
>>> +
>>> +/* Compare the keys between two entries fo the pcrel_hi hash table. */
>>> +
>>> +static int
>>> +riscv_pcrel_reloc_eq (const void *entry1, const void *entry2)
>>> +{
>>> + const struct riscv_pcrel_hi_reloc *e1 = entry1, *e2 = entry2;
>>> + return e1->address == e2->address;
>>> +}
>>> +
>>> +/* Record the pcrel_hi relocs. */
>>> +
>>> +static bool
>>> +riscv_record_pcrel_reolc (htab_t p, bfd_vma address, symbolS *symbol,
>>> + bfd_vma target)
>>> +{
>>> + struct riscv_pcrel_hi_reloc entry = { address, symbol, target };
>>> + struct riscv_pcrel_hi_reloc **slot
>>> + = (struct riscv_pcrel_hi_reloc **)htab_find_slot (p, &entry,
>>> INSERT);
>>> + if (slot == NULL)
>>> + return false;
>>> +
>>> + *slot = (struct riscv_pcrel_hi_reloc *)xmalloc (
>>> + sizeof (struct riscv_pcrel_hi_reloc));
>>> + if (*slot != NULL)
>>> + {
>>> + **slot = entry;
>>> + return true;
>>> + }
>>> + return false;
>>> +}
>>> +
>>> /* This function is called once, at assembler startup time. It should
>>> set up
>>> all the tables, etc. that the MD part of the assembler will need. */
>>>
>>> @@ -1605,6 +1657,11 @@ md_begin (void)
>>> opcode_names_hash = str_htab_create ();
>>> init_opcode_names_hash ();
>>>
>>> + /* Create pcrel_hi hash table to resolve the relocation while with
>>> + -mno-relax. */
>>> + riscv_pcrel_hi_reloc_hash
>>> + = htab_create (1024, riscv_pcrel_reloc_hash, riscv_pcrel_reloc_eq,
>>> free);
>>> +
>>> /* Set the default alignment for the text section. */
>>> record_alignment (text_section, riscv_opts.rvc ? 1 : 2);
>>> }
>>> @@ -4213,9 +4270,52 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg
>>> ATTRIBUTE_UNUSED)
>>> break;
>>>
>>> case BFD_RELOC_RISCV_PCREL_HI20:
>>> + /* record the pcrel_hi relocs of the local symbols. And evaluate
>>> the hi20
>>> + of the lcoal symbols. Fill in a tentative value to improve
>>> objdump
>>> + readability for -mrelax, and set fx_done for -mno-relax. */
>>> + if (S_IS_LOCAL (fixP->fx_addsy) && fixP->fx_addsy
>>> + && S_GET_SEGMENT (fixP->fx_addsy) == seg)
>>> + {
>>> + bfd_vma target = S_GET_VALUE (fixP->fx_addsy) + *valP;
>>> + bfd_vma value = target - md_pcrel_from (fixP);
>>> +
>>> + /* Record PCREL_HI20. */
>>> + if (!riscv_record_pcrel_reolc (riscv_pcrel_hi_reloc_hash,
>>> + md_pcrel_from (fixP),
>>> fixP->fx_addsy,
>>> + target))
>>> + {
>>> + as_warn ("too many pcrel_hi");
>>> + }
>>> +
>>> + bfd_putl32 (bfd_getl32 (buf)
>>> + | ENCODE_UTYPE_IMM (RISCV_CONST_HIGH_PART
>>> (value)),
>>> + buf);
>>> + if (!riscv_opts.relax)
>>> + fixP->fx_done = 1;
>>> + }
>>> + relaxable = riscv_opts.relax;
>>> + break;
>>> case BFD_RELOC_RISCV_PCREL_LO12_S:
>>> case BFD_RELOC_RISCV_PCREL_LO12_I:
>>> - relaxable = riscv_opts.relax;
>>> + {
>>> + /* Resolve the low12 of the local symboles with the pcrel_hi
>>> relocs.
>>> + Fill in a tentative value to improve objdump readability for
>>> + -mrelax, and set fx_done for -mno-relax. */
>>> + bfd_vma location_pcrel_hi = S_GET_VALUE (fixP->fx_addsy) + *valP;
>>> + struct riscv_pcrel_hi_reloc search = { location_pcrel_hi, 0, 0 };
>>> + struct riscv_pcrel_hi_reloc *entry
>>> + = htab_find (riscv_pcrel_hi_reloc_hash, &search);
>>> + if (entry && entry->symbol && S_IS_LOCAL (entry->symbol)
>>> + && S_GET_SEGMENT (entry->symbol) == seg)
>>> + {
>>> + bfd_vma target = entry->target;
>>> + bfd_vma value = target - entry->address;
>>> + bfd_putl32 (bfd_getl32 (buf) | ENCODE_ITYPE_IMM (value),
>>> buf);
>>> + if (!riscv_opts.relax)
>>> + fixP->fx_done = 1;
>>> + }
>>> + relaxable = riscv_opts.relax;
>>> + }
>>> break;
>>>
>>> case BFD_RELOC_RISCV_ALIGN:
>>> @@ -4980,6 +5080,15 @@ riscv_md_finish (void)
>>> riscv_set_public_attributes ();
>>> if (riscv_opts.relax)
>>> bfd_map_over_sections (stdoutput, riscv_insert_uleb128_fixes, NULL);
>>> +
>>> +}
>>> +
>>> +/* Called just before the assembler exits. */
>>> +
>>> +void
>>> +riscv_md_end (void)
>>> +{
>>> + htab_delete (riscv_pcrel_hi_reloc_hash);
>>> }
>>>
>>> /* Adjust the symbol table. */
>>> diff --git a/gas/config/tc-riscv.h b/gas/config/tc-riscv.h
>>> index 0c70c7d4739..9d2f05a4241 100644
>>> --- a/gas/config/tc-riscv.h
>>> +++ b/gas/config/tc-riscv.h
>>> @@ -80,6 +80,9 @@ extern int riscv_parse_long_option (const char *);
>>> extern void riscv_pre_output_hook (void);
>>> #define GAS_SORT_RELOCS 1
>>>
>>> +#define md_end riscv_md_end
>>> +extern void riscv_md_end (void);
>>> +
>>> /* Let the linker resolve all the relocs due to relaxation. */
>>> #define tc_fix_adjustable(fixp) 0
>>> #define md_allow_local_subtract(l,r,s) 0
>>> diff --git a/gas/testsuite/gas/riscv/fixup-local-norelax.d
>>> b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>>> new file mode 100644
>>> index 00000000000..22132a32f91
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>>> @@ -0,0 +1,23 @@
>>> +#as: -march=rv64i -mno-relax
>>> +#source: fixup-local.s
>>> +#objdump: -dr
>>> +
>>> +tmpdir/fixup-local.o: file format elf64-littleriscv
>>> +
>>> +
>>> +Disassembly of section .text:
>>> +
>>> +0+0000 <foo>:
>>> +[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <foo\+0x8>
>>> +[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+8: R_RISCV_PCREL_HI20 bar
>>> +[ ]+c:[ ]+00050513[ ]+mv a0,a0
>>> +[ ]+c: R_RISCV_PCREL_LO12_I .L0
>>> +[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+10: R_RISCV_PCREL_HI20 foo
>>> +[ ]+14:[ ]+00050513[ ]+mv a0,a0
>>> +[ ]+14: R_RISCV_PCREL_LO12_I .L0
>>> +[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <foo\+0x20>
>>> +[ ]+20:[ ]+00008067[ ]+ret
>>> diff --git a/gas/testsuite/gas/riscv/fixup-local-relax.d
>>> b/gas/testsuite/gas/riscv/fixup-local-relax.d
>>> new file mode 100644
>>> index 00000000000..49c475c5dcb
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/riscv/fixup-local-relax.d
>>> @@ -0,0 +1,41 @@
>>> +#as: -march=rv64i
>>> +#source: fixup-local.s
>>> +#objdump: -dr
>>> +
>>> +tmpdir/fixup-local.o: file format elf64-littleriscv
>>> +
>>> +
>>> +Disassembly of section .text:
>>> +
>>> +0+0000 <foo>:
>>> +[ ]+0:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+0: R_RISCV_PCREL_HI20 .LL0
>>> +[ ]+0: R_RISCV_RELAX \*ABS\*
>>> +[ ]+4:[ ]+00850513[ ]+addi a0,a0,8 # 8 <.LL0>
>>> +[ ]+4: R_RISCV_PCREL_LO12_I .L0
>>> +[ ]+4: R_RISCV_RELAX \*ABS\*
>>> +
>>> +0000000000000008 <.LL0>:
>>> +[ ]+8:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+8: R_RISCV_PCREL_HI20 bar
>>> +[ ]+8: R_RISCV_RELAX \*ABS\*
>>> +[ ]+c:[ ]+00050513[ ]+mv a0,a0
>>> +[ ]+c: R_RISCV_PCREL_LO12_I .L0
>>> +[ ]+c: R_RISCV_RELAX \*ABS\*
>>> +[ ]+10:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+10: R_RISCV_PCREL_HI20 foo
>>> +[ ]+10: R_RISCV_RELAX \*ABS\*
>>> +[ ]+14:[ ]+00050513[ ]+mv a0,a0
>>> +[ ]+14: R_RISCV_PCREL_LO12_I .L0
>>> +[ ]+14: R_RISCV_RELAX \*ABS\*
>>> +
>>> +0000000000000018 <.LL1>:
>>> +[ ]+18:[ ]+00000517[ ]+auipc a0,0x0
>>> +[ ]+18: R_RISCV_PCREL_HI20 .LL2
>>> +[ ]+18: R_RISCV_RELAX \*ABS\*
>>> +[ ]+1c:[ ]+00852503[ ]+lw a0,8\(a0\) # 20 <.LL2>
>>> +[ ]+1c: R_RISCV_PCREL_LO12_I .LL1
>>> +[ ]+1c: R_RISCV_RELAX \*ABS\*
>>> +
>>> +0000000000000020 <.LL2>:
>>> +[ ]+20:[ ]+00008067[ ]+ret
>>> diff --git a/gas/testsuite/gas/riscv/fixup-local.s
>>> b/gas/testsuite/gas/riscv/fixup-local.s
>>> new file mode 100644
>>> index 00000000000..44b47311235
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/riscv/fixup-local.s
>>> @@ -0,0 +1,13 @@
>>> +.global foo
>>> +.global bar
>>> +foo:
>>> + la a0, .LL0
>>> +.LL0:
>>> + la a0, bar
>>> + la a0, foo
>>> +.LL1:
>>> + auipc a0, %pcrel_hi(.LL2)
>>> + lw a0, %pcrel_lo(.LL1)(a0)
>>> +
>>> +.LL2:
>>> + ret
>>> --
>>> 2.39.2 (Apple Git-143)
>>>
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
2023-12-13 0:31 ` Palmer Dabbelt
@ 2023-12-13 1:26 ` Fangrui Song
0 siblings, 0 replies; 7+ messages in thread
From: Fangrui Song @ 2023-12-13 1:26 UTC (permalink / raw)
To: Palmer Dabbelt, lifang_xia; +Cc: nelson, binutils, Kito Cheng, Andrew Waterman
> In the scenario of generating .ko files, the kernel does not relax the .ko
> files. However, due to the large amount of relax and local relocation
> information, this increases the size of the .ko files.
>
> In this patch, it will finish the fixup of the local relocations while with
> "-mno-relax" option. This can reduce the size of the relocation table.
Thanks for the patch. I was initially confused by the description.
Perhaps clarify it to: "... the Linux kernel uses -mno-relax but still
gets a lot of relocations which can be eliminated" and "... resolve
some fixups to constants and remove relocations"
On Tue, Dec 12, 2023 at 4:31 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 12 Dec 2023 02:44:07 PST (-0800), nelson@rivosinc.com wrote:
> > There is another stuff, do we need to limit linkers that only can link the
> > objects with -mno-relax if this optimization applied? If so, then we
> > probably need,
> > 1. An assembler option for this optimization
> > 2. Record the object is relaxable or not into the elf attribute? or
> > readelf header? or ...
> > 2.1 Maybe we can record the relaxation information with
> > Tag_RISCV_x3_reg_usage?,
> > https://sourceware.org/pipermail/binutils/2023-September/129500.html?
> > 2.2 Once the object enables relaxation for some code, the object needs to
> > be marked as "relaxable", even if it sets `.option norelax' later.
> >
> > On Tue, Dec 12, 2023 at 5:46 PM Nelson Chu <nelson@rivosinc.com> wrote:
> >
> >> The idea looks good to me. Passed regressions of riscv-gnu-toolchain, so
> >> committed with some minor changes and indent fixes.
> >>
> >> There are some TODOs, but no rush to do for now,
> >> 1. The implementation is based on the code from bfd/elfnn-riscv.c. We
> >> probably can move the code to bfd/elfxx-riscv.c, so that can reduce
> >> duplicate code, just like what we did for the architecture parser.
> >> Before that, I renamed functions and variables from *reloc* to *fixup*, to
> >> distinguish the code from bfd/elfnn-riscv.c, since they are still a little
> >> bit different.
> >> 2. Maybe not only pcrel_hi/lo12 relocation with local symbols can be resolved
> >> at assembler time. Other pc-relative relocation, like branch, may also
> >> be able to perform related optimizations.
Agree that GNU assembler can resolve more PC-relative instructions to
a constant without a relocation.
For example, LLVM integrated assembler resolves the following `j` to constants:
// clang --target=riscv64-linux-gnu -march=rv64i -mno-relax -c
j .Ltmp1
j .Ltmp1
.Ltmp1:
(Though LLVM has its own problem that a `.option relax` anywhere in
the assembly file will lose this optimization.)
> Nelson and I were just talking about this. I'd been leaning towards
> adding another option along the lines of "-mno-relax-abi", which would
> explicitly mean that users can depend on no objects being relaxed. That
> said, I'm not actually sure I can come up with a case where anything
> breaks.
>
> I was specifically worried about things like the compiler doing label
> subtraction, but IIUC that can only happen within a single translation
> unit so we're safe to take advantage of on R_RISCV_RELAX relocations.
>
> There's also cases like misaligned globals, but we're already broken
> there so I'm not sure it counts.
>
> So maybe this safe?
I lean towards not providing an option. Beside the previous paragraph
that LLVM integrated assembler omits relocations in more cases,
architectures not using linker relaxation do optimize out relocations
in these cases as well.
It's a missing size optimization for RISC-V in the -mno-relax configuration.
Users relying on the relocation can switch to `.reloc ., R_RISCV_JAL, xx`.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
2023-12-13 2:47 ` Palmer Dabbelt
@ 2023-12-13 3:10 ` Palmer Dabbelt
0 siblings, 0 replies; 7+ messages in thread
From: Palmer Dabbelt @ 2023-12-13 3:10 UTC (permalink / raw)
To: i; +Cc: lifang_xia, nelson, binutils, Kito Cheng, Andrew Waterman
On Tue, 12 Dec 2023 18:47:05 PST (-0800), Palmer Dabbelt wrote:
> On Tue, 12 Dec 2023 17:26:03 PST (-0800), i@maskray.me wrote:
>>> In the scenario of generating .ko files, the kernel does not relax the .ko
>>> files. However, due to the large amount of relax and local relocation
>>> information, this increases the size of the .ko files.
>>>
>>> In this patch, it will finish the fixup of the local relocations while with
>>> "-mno-relax" option. This can reduce the size of the relocation table.
>>
>> Thanks for the patch. I was initially confused by the description.
>> Perhaps clarify it to: "... the Linux kernel uses -mno-relax but still
>> gets a lot of relocations which can be eliminated" and "... resolve
>> some fixups to constants and remove relocations"
>>
>> On Tue, Dec 12, 2023 at 4:31 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>
>>> On Tue, 12 Dec 2023 02:44:07 PST (-0800), nelson@rivosinc.com wrote:
>>> > There is another stuff, do we need to limit linkers that only can link the
>>> > objects with -mno-relax if this optimization applied? If so, then we
>>> > probably need,
>>> > 1. An assembler option for this optimization
>>> > 2. Record the object is relaxable or not into the elf attribute? or
>>> > readelf header? or ...
>>> > 2.1 Maybe we can record the relaxation information with
>>> > Tag_RISCV_x3_reg_usage?,
>>> > https://sourceware.org/pipermail/binutils/2023-September/129500.html?
>>> > 2.2 Once the object enables relaxation for some code, the object needs to
>>> > be marked as "relaxable", even if it sets `.option norelax' later.
>>> >
>>> > On Tue, Dec 12, 2023 at 5:46 PM Nelson Chu <nelson@rivosinc.com> wrote:
>>> >
>>> >> The idea looks good to me. Passed regressions of riscv-gnu-toolchain, so
>>> >> committed with some minor changes and indent fixes.
>>> >>
>>> >> There are some TODOs, but no rush to do for now,
>>> >> 1. The implementation is based on the code from bfd/elfnn-riscv.c. We
>>> >> probably can move the code to bfd/elfxx-riscv.c, so that can reduce
>>> >> duplicate code, just like what we did for the architecture parser.
>>> >> Before that, I renamed functions and variables from *reloc* to *fixup*, to
>>> >> distinguish the code from bfd/elfnn-riscv.c, since they are still a little
>>> >> bit different.
>>> >> 2. Maybe not only pcrel_hi/lo12 relocation with local symbols can be resolved
>>> >> at assembler time. Other pc-relative relocation, like branch, may also
>>> >> be able to perform related optimizations.
>>
>> Agree that GNU assembler can resolve more PC-relative instructions to
>> a constant without a relocation.
>> For example, LLVM integrated assembler resolves the following `j` to constants:
>>
>> // clang --target=riscv64-linux-gnu -march=rv64i -mno-relax -c
>> j .Ltmp1
>> j .Ltmp1
>> .Ltmp1:
>>
>> (Though LLVM has its own problem that a `.option relax` anywhere in
>> the assembly file will lose this optimization.)
>>
>>> Nelson and I were just talking about this. I'd been leaning towards
>>> adding another option along the lines of "-mno-relax-abi", which would
>>> explicitly mean that users can depend on no objects being relaxed. That
>>> said, I'm not actually sure I can come up with a case where anything
>>> breaks.
>>>
>>> I was specifically worried about things like the compiler doing label
>>> subtraction, but IIUC that can only happen within a single translation
>>> unit so we're safe to take advantage of on R_RISCV_RELAX relocations.
>>>
>>> There's also cases like misaligned globals, but we're already broken
>>> there so I'm not sure it counts.
>>>
>>> So maybe this safe?
>>
>> I lean towards not providing an option. Beside the previous paragraph
>> that LLVM integrated assembler omits relocations in more cases,
>> architectures not using linker relaxation do optimize out relocations
>> in these cases as well.
>> It's a missing size optimization for RISC-V in the -mno-relax configuration.
>> Users relying on the relocation can switch to `.reloc ., R_RISCV_JAL, xx`.
>
> Sorry for being confusing: I agree resolving local R_RISCV_CALL
> relocations at assembly time (or I guess, not having R_RISCV_CALL at
> all) is safe as long as the call does not cross a R_RISCV_RELAX (or any
> of the normal-smelling boundaries like sections and such).
>
> Nelson and I were wondering if many other PC-relative relocations can be
> processed at assembly time whenever they don't cross a R_RISCV_RELAX
I guess that's a silly statement: of course we can do all the
assembler-time processing. I was mixing up compile-time stuff like
working around the auipc overflows in there -- I think we can also do
that sort of stuff, I just forgot there was anything going on before the
assembler.
Probably time to get some sleep...
> boundary (and meet all the other rules the other ports need to deal
> with). I think there's a bunch where we can, and it's possible just
> having no relaxations in an object is sufficient to enable any of these
> optimizations.
>
> FWIW, I'd still lean towards providing some sort of option/ELF flag that
> just bans relaxation entirely -- we basically don't use it in modern
> Linux user binaries (unless I missed some PIE optimizations going in),
> so we might as well just properly forbid relaxation.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax"
[not found] <DS7PR12MB5765B60BD9651EB35F5FFACECB8DA@DS7PR12MB5765.namprd12.prod.outlook.com>
@ 2023-12-13 2:47 ` Palmer Dabbelt
2023-12-13 3:10 ` Palmer Dabbelt
0 siblings, 1 reply; 7+ messages in thread
From: Palmer Dabbelt @ 2023-12-13 2:47 UTC (permalink / raw)
To: i; +Cc: lifang_xia, nelson, binutils, Kito Cheng, Andrew Waterman
On Tue, 12 Dec 2023 17:26:03 PST (-0800), i@maskray.me wrote:
>> In the scenario of generating .ko files, the kernel does not relax the .ko
>> files. However, due to the large amount of relax and local relocation
>> information, this increases the size of the .ko files.
>>
>> In this patch, it will finish the fixup of the local relocations while with
>> "-mno-relax" option. This can reduce the size of the relocation table.
>
> Thanks for the patch. I was initially confused by the description.
> Perhaps clarify it to: "... the Linux kernel uses -mno-relax but still
> gets a lot of relocations which can be eliminated" and "... resolve
> some fixups to constants and remove relocations"
>
> On Tue, Dec 12, 2023 at 4:31 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Tue, 12 Dec 2023 02:44:07 PST (-0800), nelson@rivosinc.com wrote:
>> > There is another stuff, do we need to limit linkers that only can link the
>> > objects with -mno-relax if this optimization applied? If so, then we
>> > probably need,
>> > 1. An assembler option for this optimization
>> > 2. Record the object is relaxable or not into the elf attribute? or
>> > readelf header? or ...
>> > 2.1 Maybe we can record the relaxation information with
>> > Tag_RISCV_x3_reg_usage?,
>> > https://sourceware.org/pipermail/binutils/2023-September/129500.html?
>> > 2.2 Once the object enables relaxation for some code, the object needs to
>> > be marked as "relaxable", even if it sets `.option norelax' later.
>> >
>> > On Tue, Dec 12, 2023 at 5:46 PM Nelson Chu <nelson@rivosinc.com> wrote:
>> >
>> >> The idea looks good to me. Passed regressions of riscv-gnu-toolchain, so
>> >> committed with some minor changes and indent fixes.
>> >>
>> >> There are some TODOs, but no rush to do for now,
>> >> 1. The implementation is based on the code from bfd/elfnn-riscv.c. We
>> >> probably can move the code to bfd/elfxx-riscv.c, so that can reduce
>> >> duplicate code, just like what we did for the architecture parser.
>> >> Before that, I renamed functions and variables from *reloc* to *fixup*, to
>> >> distinguish the code from bfd/elfnn-riscv.c, since they are still a little
>> >> bit different.
>> >> 2. Maybe not only pcrel_hi/lo12 relocation with local symbols can be resolved
>> >> at assembler time. Other pc-relative relocation, like branch, may also
>> >> be able to perform related optimizations.
>
> Agree that GNU assembler can resolve more PC-relative instructions to
> a constant without a relocation.
> For example, LLVM integrated assembler resolves the following `j` to constants:
>
> // clang --target=riscv64-linux-gnu -march=rv64i -mno-relax -c
> j .Ltmp1
> j .Ltmp1
> .Ltmp1:
>
> (Though LLVM has its own problem that a `.option relax` anywhere in
> the assembly file will lose this optimization.)
>
>> Nelson and I were just talking about this. I'd been leaning towards
>> adding another option along the lines of "-mno-relax-abi", which would
>> explicitly mean that users can depend on no objects being relaxed. That
>> said, I'm not actually sure I can come up with a case where anything
>> breaks.
>>
>> I was specifically worried about things like the compiler doing label
>> subtraction, but IIUC that can only happen within a single translation
>> unit so we're safe to take advantage of on R_RISCV_RELAX relocations.
>>
>> There's also cases like misaligned globals, but we're already broken
>> there so I'm not sure it counts.
>>
>> So maybe this safe?
>
> I lean towards not providing an option. Beside the previous paragraph
> that LLVM integrated assembler omits relocations in more cases,
> architectures not using linker relaxation do optimize out relocations
> in these cases as well.
> It's a missing size optimization for RISC-V in the -mno-relax configuration.
> Users relying on the relocation can switch to `.reloc ., R_RISCV_JAL, xx`.
Sorry for being confusing: I agree resolving local R_RISCV_CALL
relocations at assembly time (or I guess, not having R_RISCV_CALL at
all) is safe as long as the call does not cross a R_RISCV_RELAX (or any
of the normal-smelling boundaries like sections and such).
Nelson and I were wondering if many other PC-relative relocations can be
processed at assembly time whenever they don't cross a R_RISCV_RELAX
boundary (and meet all the other rules the other ports need to deal
with). I think there's a bunch where we can, and it's possible just
having no relaxations in an object is sufficient to enable any of these
optimizations.
FWIW, I'd still lean towards providing some sort of option/ELF flag that
just bans relaxation entirely -- we basically don't use it in modern
Linux user binaries (unless I missed some PIE optimizations going in),
so we might as well just properly forbid relaxation.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-12-13 3:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-29 9:17 [PATCH] RISC-V: Do fixup for local symbols while with "-mno-relax" lifang_xia
2023-12-12 9:46 ` Nelson Chu
2023-12-12 10:44 ` Nelson Chu
2023-12-13 0:31 ` Palmer Dabbelt
2023-12-13 1:26 ` Fangrui Song
[not found] <DS7PR12MB5765B60BD9651EB35F5FFACECB8DA@DS7PR12MB5765.namprd12.prod.outlook.com>
2023-12-13 2:47 ` Palmer Dabbelt
2023-12-13 3:10 ` Palmer Dabbelt
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).