[PATCH] D138135: [WIP][ELF] Support LoongArch

[Jinyang He via Phabricator via llvm-commits]

MQ-mengqing added inline comments.


================
Comment at: lld/ELF/Arch/LoongArch.cpp:322
+  write32le(buf + 8, insn(JIRL, R_T1, R_T3, 0));
+  write32le(buf + 12, insn(ANDI, 0, 0, 0));
+}
----------------
The insn is correct, but I think it's better to use something like R_ZERO rather than 0. Of course, you can ignore this comment if you think both are OK.


================
Comment at: lld/ELF/Arch/LoongArch.cpp:375
+  case R_LARCH_GOT64_PC_LO20:
+  case R_LARCH_GOT64_PC_HI12:
+    return R_LOONGARCH_GOT_PAGE_PC;
----------------
The TLSGD and TLSLD also override the GOT64, unlikely. To be honest, I'm also confused by the relocation design.
a, R_LARCH_TLS_{LD, GD}_PC_HI20 + R_LARCH_GOT_PC_LO12 + R_LARCH_GOT64_PC_LO20 + R_LARCH_GOT64_PC_HI12,
b, R_LARCH_TLS_{LD, GD}_HI20 + R_LARCH_GOT_LO12, R_LARCH_GOT64_LO20, R_LARCH_GOT64_HI12.
If only one RelExpr used by low-12bits is added, will it be inadequate.



================
Comment at: lld/ELF/InputSection.cpp:653
+    // duplicate some logic here.
+    if (sym.isTls()) {
+      if (sym.hasFlag(NEEDS_TLSGD))
----------------
How about the "la.tls.ie" and "la.tls.gd" do the same symbol?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138135