Re: [PATCH] RISC-V: emit R_RISCV_RELAX for the la pseudo instruction

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 6670 bytes --]

On Tue, Sep 19, 2023 at 3:03 PM Rui Ueyama via Binutils <
binutils@sourceware.org> wrote:

> Some psABIs define a relaxation to turn a GOT load into a PC-relative
> address materialization. For example, the AArch64's psABI allows
> adrp+ldr to be rewritten to nop+adr to eliminate the memory load.
> This patch is part of the effort to make such optimization possible
> for RISC-V.
>
> For RISC-V, we use the la assembly pseudo instruction to load a symbol
> address from the GOT. The pseudo instruction is expanded to auipc+ld.
> If the address loaded by the instruction pair is actually a
> PC-relative link-time constant, we want the linker to rewrite the
> instruction pair with auipc+addi.
>
> We can't rewrite all existing auipc+ld pairs with auipc+addi in the
> linker because there might be code that jumps to the middle of the
> instruction pair. That should be extremely rare, if ever exists, but
> you can at least in theory write a program in assembly that jumps to
> the ld instruction of the instruction pair. We need a marker to
> identify that an auipc+ld can be safely relaxed (i.e. they are emitted
> for la).
>

Make sense.


> This patch is to annotate R_RISCV_GOT_HI20 with R_RISCV_RELAX only
> when the relocation is emitted for the la pseudo instruction. The
> linker will use it as a signal that the instruction pair can be safely
> relaxed.
>

Currently GNU ld won't do any relaxations for GOT patterns, so we will also
need the related updates in the future.  Besides, not sure if the new GOT
relaxations will break this feature for undefined weak symbol,
https://github.com/bminor/binutils-gdb/commit/9d1da81b261a20050ef2ad01a5b4c8cf78404222.
But fortunately if we just mark the R_RISCV_RELAX for GOT_HI20 in
assembler, that shouldn't cause a problem since currently ld will ignore it
for now.


> Proposal to the RISC-V psABI:
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/397
> ---
>  bfd/bfd-in2.h                         | 1 +
>  bfd/elfxx-riscv.c                     | 1 +
>  gas/config/tc-riscv.c                 | 3 ++-
>  gas/testsuite/gas/riscv/la-variants.d | 3 +++
>  4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> index 1c4f75ae244..e15641a1d00 100644
> --- a/bfd/bfd-in2.h
> +++ b/bfd/bfd-in2.h
> @@ -5413,6 +5413,7 @@ number for the SBIC, SBIS, SBI and CBI instructions
> */
>    BFD_RELOC_RISCV_SUB32,
>    BFD_RELOC_RISCV_SUB64,
>    BFD_RELOC_RISCV_GOT_HI20,
> +  BFD_RELOC_RISCV_GOT_HI20_RELAX,
>    BFD_RELOC_RISCV_TLS_GOT_HI20,
>    BFD_RELOC_RISCV_TLS_GD_HI20,
>    BFD_RELOC_RISCV_JMP,
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 6ed657171f0..71e63e7b789 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -913,6 +913,7 @@ static const struct elf_reloc_map riscv_reloc_map[] =
>    { BFD_RELOC_RISCV_PCREL_HI20, R_RISCV_PCREL_HI20 },
>    { BFD_RELOC_RISCV_JMP, R_RISCV_JAL },
>    { BFD_RELOC_RISCV_GOT_HI20, R_RISCV_GOT_HI20 },
> +  { BFD_RELOC_RISCV_GOT_HI20_RELAX, R_RISCV_GOT_HI20 },
>    { BFD_RELOC_RISCV_TLS_DTPMOD32, R_RISCV_TLS_DTPMOD32 },
>    { BFD_RELOC_RISCV_TLS_DTPREL32, R_RISCV_TLS_DTPREL32 },
>    { BFD_RELOC_RISCV_TLS_DTPMOD64, R_RISCV_TLS_DTPMOD64 },
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 3b520ad208b..303ae18436c 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -2039,7 +2039,7 @@ macro (struct riscv_cl_insn *ip, expressionS
> *imm_expr,
>        else if ((riscv_opts.pic && mask == M_LA)
>                || mask == M_LGA)
>         pcrel_load (rd, rd, imm_expr, LOAD_ADDRESS_INSN,
> -                   BFD_RELOC_RISCV_GOT_HI20,
> BFD_RELOC_RISCV_PCREL_LO12_I);
> +                   BFD_RELOC_RISCV_GOT_HI20_RELAX,
> BFD_RELOC_RISCV_PCREL_LO12_I);
>        /* Local PIC symbol, or any non-PIC symbol.  */
>        else
>         pcrel_load (rd, rd, imm_expr, "addi",
> @@ -4244,6 +4244,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg
> ATTRIBUTE_UNUSED)
>        relaxable = true;
>        break;
>
> +    case BFD_RELOC_RISCV_GOT_HI20_RELAX:

     case BFD_RELOC_RISCV_PCREL_HI20:
>      case BFD_RELOC_RISCV_PCREL_LO12_S:
>      case BFD_RELOC_RISCV_PCREL_LO12_I:
>

In the gas/write.h, there is a special hook called tc_fix_data, which
allows backends to attach their own data to the fixup structure.  The fixup
structure is used to represent the relocation in assembler.  Therefore, I
suggest that we should add a new flag in the tc_fix_data to represent the
relocations which are expanded from the macro, setup the flag in the
macro_build function, and then enable to attach the R_RISCV_RELAX for
GOT_HI20 if it is expanded from the macro LA/LGA by checking that fag.  The
advantage of doing this is that we don't need to add a new special bfd
relocation for the pseudo LA, and the similar usage in the future also will
not need it.

Thanks
Nelson


> diff --git a/gas/testsuite/gas/riscv/la-variants.d
> b/gas/testsuite/gas/riscv/la-variants.d
> index b1d316983b7..e8ac09c2af2 100644
> --- a/gas/testsuite/gas/riscv/la-variants.d
> +++ b/gas/testsuite/gas/riscv/la-variants.d
> @@ -21,11 +21,13 @@ Disassembly of section .text:
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
>  [      ]+[0-9a-f]+:[   ]+00000617[     ]+auipc[        ]+a2,0x0
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_GOT_HI20[     ]+a
> +[      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
>  [      ]+[0-9a-f]+:[   ]+(00062603|00063603)[  ]+(lw|ld)[
> ]+a2,0\(a2\).*
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_PCREL_LO12_I[         ]+\.L0[ ]+
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
>  [      ]+[0-9a-f]+:[   ]+00000697[     ]+auipc[        ]+a3,0x0
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_GOT_HI20[     ]+a
> +[      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
>  [      ]+[0-9a-f]+:[   ]+(0006a683|0006b683)[  ]+(lw|ld)[
> ]+a3,0\(a3\).*
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_PCREL_LO12_I[         ]+\.L0[ ]+
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
> @@ -37,6 +39,7 @@ Disassembly of section .text:
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
>  [      ]+[0-9a-f]+:[   ]+00000797[     ]+auipc[        ]+a5,0x0
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_GOT_HI20[     ]+a
> +[      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
>  [      ]+[0-9a-f]+:[   ]+(0007a783|0007b783)[  ]+(lw|ld)[
> ]+a5,0\(a5\).*
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_PCREL_LO12_I[         ]+\.L0[ ]+
>  [      ]+[0-9a-f]+:[   ]+R_RISCV_RELAX[        ]+\*ABS\*
> --
> 2.34.1
>
>