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

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n 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));
+}
----------------
MQ-mengqing wrote:
> 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.
> 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.

This is very reasonable as it's huge readability improvement, I'll fix in next push.


================
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;
----------------
MQ-mengqing wrote:
> 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.
> 
Holy ****. Of course I'll adjust later. Thanks for sharing this precious insight, I can't by any chance notice this due to being occupied by `$DAY_JOB` lately.


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

What do you mean by "do the same symbol" here? Maybe you can tell me in a bit of Chinese if it's easier to explain that way... for now honestly I can't understand. (If you do attach some Chinese phrase I'll take care of translating it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138135