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

Fri Jul 7 14:20:50 PDT 2023

MaskRay added inline comments.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:22
+
+// The LoongArch `pcalau12i` is behaviorally equivalent to the AArch64 `adrp`,
+// so a similar concept of "page" also applies here. A "page" is in fact just
----------------
This can be rephrase to mention what a page is first, then mention that behaviorally equivalent to the AArch64 `adrp`. Ideally understanding an arch doesn't require understanding another independent one.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:28
+// Note: "pcalau12i" stands for something like "PC aligned add upper, from 12th
+// bit, immediate". (The official ISA manual gives no such "full names".)
+static uint64_t getLoongArchPage(uint64_t p) {
----------------
" (The official ISA manual gives no such "full names".)" this probably should be dropped. The official ISA manual can improve and I don't hope that we push another commit to fix this.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:29
+// bit, immediate". (The official ISA manual gives no such "full names".)
+static uint64_t getLoongArchPage(uint64_t p) {
+  return p & ~static_cast<uint64_t>(0xfff);
----------------
It'd be better to define the helpers after `class LoongArch`.
The AArch64 style in this particular place is something we'd not do.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:37
+// as negative, because the instructions consuming it (ld, st, addi, etc.)
+// all sign-extend the immediate, unlike AArch64. The higher bits need
+// tweaking too, due to potential usage in patterns like:
----------------
"The higher bits need ..." appears to describe `result -= 0x100000000` but is not clear. An example will work better

================
Comment at: lld/ELF/Arch/LoongArch.cpp:155
+  defaultMaxPageSize = 65536;
+  write32(trapInstr.data(), BREAK); // break 0
+
----------------
write32le

================
Comment at: lld/ELF/Arch/LoongArch.cpp:194
+
+  for (auto *sec : f->getSections()) {
+    if (sec && (sec->flags & SHF_EXECINSTR))
----------------
delete braces 

================
Comment at: lld/ELF/Arch/LoongArch.cpp:224
+      error(toString(f) +
+            ": cannot link object files with different ABI");
+
----------------
mention `toString(ctx->objectFiles[0])` . See D134198

================
Comment at: lld/ELF/Arch/LoongArch.cpp:394
+    // [1]: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=9f482b73f41a9a1bbfb173aad0733d1c824c788a
+    // [2]: https://github.com/loongson/LoongArch-Documentation/pull/69
+    return isJirl(read32le(loc)) ? R_PLT : R_ABS;
----------------
This link gives "This repository has been archived by the owner on Apr 20, 2023. It is now read-only.". Is it stale?

Is R_LARCH_JIRL_LO12 still planned?

================
Comment at: lld/ELF/Arch/LoongArch.cpp:437
+    return R_LOONGARCH_TLSGD_PAGE_PC;
+  case R_LARCH_PCALA_HI20:
+    // Why not R_LOONGARCH_PAGE_PC, majority of references don't go through PLT
----------------
It seems that https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html  is wrong about `GR[rd] = PC + SignExtend({si20, 12'b0}, GRLEN)`.

The low 12-bits of GR[rd] is zeroed.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:451
+    // apart that relationship is not certain anymore), and programmer mistakes
+    // (e.g. as outlined in https://github.com/loongson/LoongArch-Documentation/pull/69).
+    //
----------------
https://github.com/loongson/LoongArch-Documentation/pull/69 is now a stale link as the repo is archived and may be deleted in the future.

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:8
+
+# RUN: ld.lld %t/a.la32.o -T %t/case1.t -o %t/case1.la32
+# RUN: ld.lld %t/a.la64.o -T %t/case1.t -o %t/case1.la64
----------------
You can use something like `--section-start .rodata=0x11000 --section-start=.text=0x11ffc` to replace the linker script.

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:20
+
+# RUN: ld.lld %t/a.la32.o -T %t/case2.t -o %t/case2.la32
+# RUN: ld.lld %t/a.la64.o -T %t/case2.t -o %t/case2.la64
----------------
I hope that we can reduce the number of ld.lld invocations. 

Does it help to increase the number of instructions in `.text`?
```
  pcalau12i $a0, %pc_hi20(x)
  pcalau12i $a0, %pc_hi20(x)
  ld.w      $a0, $a0, %pc_lo12(x)
  ld.w      $a0, $a0, %pc_lo12(x)
```

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:130
+# RUN: llvm-objdump -d --no-show-raw-insn %t/extreme2 | FileCheck %s --check-prefix=EXTREME2
+# EXTREME2:      addi.d    $t0, $zero, -820
+# EXTREME2-NEXT: lu32i.d   $t0, -69907
----------------
This is complex. This needs a comment to explain how these immediate mean.

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:169
+_start:
+  addi.d    $t0, $zero, %pc_lo12(x)
+  lu32i.d   $t0, %pc64_lo20(x)
----------------
You can define a macro to repeat the code sequence 3 times so that we don't need many `extreme*.t`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138135