[PATCH] D143673: [lld][RISCV] Implement GP relaxation for R_RISCV_HI20/R_RISCV_LO12_I/R_RISCV_LO12_S.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

This patch seems mostly ready and should make D147983 <https://reviews.llvm.org/D147983> unneeded. See D147983 <https://reviews.llvm.org/D147983> that some users are waiting as well.



================
Comment at: lld/ELF/Arch/RISCV.cpp:53
+// of the psABI spec, but match the values binutils uses internally.
+#define INTERNAL_R_RISCV_GPREL_I 47
+#define INTERNAL_R_RISCV_GPREL_S 48
----------------
Don't use the binutils numbers which may conflict with future relocation types.

For now, just use numbers starting with 256, which are unavailable for ELFCLASS32.


================
Comment at: lld/ELF/Arch/RISCV.cpp:649
+  case R_RISCV_HI20:
+    // delete unnecessary instruction
+    sec.relaxAux->relocTypes[i] = R_RISCV_RELAX;
----------------
Remove this comment or be more specific about which instruction is to be removed. See the `case R_RISCV_TPREL_ADD` example.


================
Comment at: lld/ELF/Options.td:359
+defm relax_gp: BB<"relax-gp",
+  "Enable GP relaxation",
+  "Disable GP relaxation (default)">;
----------------
It seems that "global pointer relaxation" is the canonical term. Use it instead of "GP relaxation".


================
Comment at: lld/ELF/Writer.cpp:1861
       OutputSection *sec = findSection(".sdata");
-      addOptionalRegular("__global_pointer$", sec ? sec : Out::elfHeader, 0x800,
-                         STV_DEFAULT);
+      Defined *globalPointer = addOptionalRegular(
+          "__global_pointer$", sec ? sec : Out::elfHeader, 0x800, STV_DEFAULT);
----------------
Joe wrote:
> If the global pointer has been defined in the linkerscript, addOptionalRegular will return nullptr and ElfSym::riscvGlobalPointer won't be set. Is it worth adding a check for this and setting it if the symbol is defined?
See @Joe 's comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143673/new/

https://reviews.llvm.org/D143673